Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enableBuildNotifications should not be enabled by default #1161

Open
jennevdmeer opened this issue Oct 26, 2022 · 7 comments · May be fixed by symfony/recipes#1353
Open

enableBuildNotifications should not be enabled by default #1161

jennevdmeer opened this issue Oct 26, 2022 · 7 comments · May be fixed by symfony/recipes#1353

Comments

@jennevdmeer
Copy link
Contributor

Why does default encore come with enableBuildNotifications on by default. Maybe it's my hate for notifications but In general I feel like things should not just install/run "random" binaries by default.

It comes from ..\encore-test\node_modules\node-notifier\vendor\snoreToast\snoretoast-x64.exe. And for me on windows it even created a shortcut in my start menu without any consent. While at the same time not removing it when I remove encore (or npm's node-notifier).

I feel like the consequences of that enabled should be clearly explained in the config and manually be enabled.

@jennevdmeer jennevdmeer changed the title enableBuildNotifications should not be on by default enableBuildNotifications should not be enabled by default Oct 26, 2022
@Kocal
Copy link
Member

Kocal commented Oct 26, 2022

I'm also in favor in this, I've always removed the .enableBuildNotifications() from the configuration file and the webpack-notifier dependency.

I never found those notifications very useful, but that's just my opinion.

@weaverryan
Copy link
Member

@carsonbot
Copy link

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

@jennevdmeer
Copy link
Contributor Author

No

@stof
Copy link
Member

stof commented Nov 5, 2024

@jennevdmeer are you willing to contribute to remove this from the recipe (see the links in @weaverryan's comment above for the places needing to be changed) ?

Edit: those links go to the recipe for the 1.10 version, while changes now should be done in the recipe for the highest version, which means the 2.0 recipe now (or in all versions)

@jennevdmeer
Copy link
Contributor Author

@stof I could, thought about it with the new notice. But I wanted to create a similar message to those "enableSassLoader requires sass use npm i sass sass-loader --save-dev" like messages when you would enable it and start the dev server. Which I would assume would be a good idea to add for this too then. I wanted to look at it but haven't had the time to figure that out yet.

If you think the message is not needed I can just do it tomorrow'ish.

@stof
Copy link
Member

stof commented Nov 6, 2024

@jennevdmeer Encore already makes the build notifications optional (and disabled by default), with such a message if the dependency is missing.
build notifications are only enabled by default in the recipe, not in Encore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants