search

symfony / webpack-encore

A simple but powerful API for processing & compiling assets built around Webpack
https://symfony.com/doc/current/frontend.html
MIT License
2.23k stars 198 forks source link

`enableBuildNotifications` should not be enabled by default #1161

Open jennevdmeer opened 2 years ago

jennevdmeer commented 2 years ago

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.

Kocal commented 2 years ago

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 commented 2 years ago

I'm fine with disabling by default - it would be over in the recipe - https://github.com/symfony/recipes/blob/main/symfony/webpack-encore-bundle/1.10/webpack.config.js - we could also then remove https://github.com/symfony/recipes/blob/main/symfony/webpack-encore-bundle/1.10/package.json#L12

Cheers!

carsonbot commented 3 days ago

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

jennevdmeer commented 1 day ago

No

stof commented 1 day ago

@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 commented 3 hours ago

@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 commented 1 hour ago

@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.