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

Make webpack's done hook wait until analyzer writes report / stat file. #247

Merged

Conversation

mareolan
Copy link
Contributor

@mareolan mareolan commented Feb 2, 2019

Fixes #232 by prolonging webpack's "done" hook until analyzer finishes writing report / stat files.

@jsf-clabot
Copy link

jsf-clabot commented Feb 2, 2019

CLA assistant check
All committers have signed the CLA.

@liangchunn
Copy link

Nice work! This fixes the issue in my reproducible repo mentioned in #232.

actions.forEach(action => action());
setImmediate(async () => {
try {
for (let i = 0, len = actions.length; i < len; ++i) await actions[i]();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to pre-optimise the loop here. AFAIK V8's optimising compiler does this for us automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean write it as for (let i = 0; i < actions.length; ... instead?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, IMO for(let i = 0; i < actions.length; i++) more readable than for(let i = 0, len = actions.length; i < len; ++i ), plus it wouldn't make a huge difference in terms of perf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, no problem.

@valscion
Copy link
Member

This is looking good to me, thanks! Have you checked which version of webpack has the tapAsync API available, i.e. is it available in webpack 3 or only in 4 and up?

@mareolan
Copy link
Contributor Author

tapAsync is available only for webpack >= 4.0.0. I tried the repro also with latest 3.12.0 and since there's no compiler.hooks, it uses the alternative code flow and webpack won't wait for bundle analyzer. So basically, this fix fixes it only for webpack >= 4.0.0.

src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
src/BundleAnalyzerPlugin.js Outdated Show resolved Hide resolved
@valscion valscion merged commit 1c8aba3 into webpack-contrib:master Feb 12, 2019
@valscion valscion mentioned this pull request Feb 12, 2019
@th0r
Copy link
Collaborator

th0r commented Feb 12, 2019

@mareolan thanks a lot!

@valscion
Copy link
Member

This is now released in v3.0.4. Thank you for your contributions!

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 this pull request may close these issues.

5 participants