markshapiro / webpack-merge-and-include-globally

Merge multiple files (js, css..) and include by running them "as is". Supports minify by custom transform and wildcard paths.
MIT License
102 stars 27 forks source link

Webpack watch mode causes stacking of callbacks #61

Closed bytasv closed 3 years ago

bytasv commented 3 years ago

Hello,

I'm using this plugin on my project and I noticed a weird thing happening, when I run my code in watch mode webpack --watch I noticed that callback function as second argument to this plugin is being called more and more times every time changes are detected and everything is recompiled. I.e. if I just started in watch mode, then it will be run once, if I then make a change it will be ran twice, and so on... so if I keep working in watch mode it will just keep growing and executing same thing over and over again increasing with each recompile.

I managed to narrow it down to this line - my guess is that compiler.hooks.thisCompilation.tap will keep binding and not releasing previously bound events so they just keep stacking.

If I remove this line and modify following lines like this:

if (compiler.hooks.processAssets) {
  compiler.hooks.processAssets.tapAsync({
    name: plugin.name,
    stage: Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL,
  },
  (_, callback) => this.run(compilation, callback));
} else {
  compiler.hooks.emit.tapAsync(plugin.name, this.run.bind(this));
}

It seems to be working as I expect - it only runs once per compilation. I couldn't understand why compiler.hooks.thisCompilation.tap was needed and would such change continue to work as expected?

I'm happy to open PR with this change but I'm not sure if I understand everything correctly.

Thank you

markshapiro commented 3 years ago

@bytasv i copied webpack 5 adjustments from https://github.com/FormidableLabs/webpack-stats-plugin/blob/main/lib/stats-writer-plugin.js#L78 together with compiler.hooks.thisCompilation.tap without understanding in depth.

i cant not reproduce this if i run it locally on example inside the plugin doing node_modules/.bin/webpack --watch, maybe you have a different webpack version than in local devDependencies?

Im thinking maybe i should fix it by using official documentation of WP plugins https://webpack.js.org/contribute/writing-a-plugin/

markshapiro commented 3 years ago

@bytasv your code change is giving me this warning: BREAKING CHANGE: No more changes should happen to Compilation.assets after sealing the Compilation. Do changes to assets earlier, e. g. in Compilation.hooks.processAssets. Make sure to select an appropriate stage from Compilation.PROCESS_ASSETS_STAGE_*.

can you check if this code will fix it for you? (using promise instead of async, maybe promise will make it call once)

compiler.hooks.thisCompilation.tap(
        plugin.name,
        (compilation) => {
            compilation.hooks.processAssets.tapPromise(
              {
                name: plugin.name,
                stage: Compilation.PROCESS_ASSETS_STAGE_ADDITIONAL,
              },
              () => new Promise((resolve, reject) => this.run(compilation, resolve))
            );
        },
      );

if not then ill go with your code

bytasv commented 3 years ago

hey there, thanks for looking into it!

Sorry, my bad - I didn't mention that I'm running webpack 4 - so maybe that's the issue?

However what I found does also work for me, is if I simply skip checking if compiler.hooks and just use:

compiler.plugin('emit', this.run.bind(this))

https://github.com/markshapiro/webpack-merge-and-include-globally/blob/master/index.js#L40

It seems that I end up in the branch which is probably suppose to work for webpack 5? But that line above seems to work for me as well 🤷

markshapiro commented 3 years ago

@bytasv fixed when calling compiler.hooks.emit.tapAsync once only

bytasv commented 3 years ago

Thanks alot @markshapiro !!! It's working as expected now 🙇