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

fix(core): close webpack compiler when build complete #6608

Conversation

kazamov
Copy link
Contributor

@kazamov kazamov commented Aug 5, 2021

Call webpack close method when build complete according to . Fallback webpack 4 close method

Current Behavior

Currently the webpack compiler does not call the close method after the build complete, because webpack 4 does not have it. But it is crucial for the webpack 5 to be able to use persistent cache.

Expected Behavior

The close method of webpack compiler instance called when build complete. Fallback webpack 4 compiler instance with the close method.

Zakir Nuriiev added 2 commits August 5, 2021 10:16
Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run. Fallback webpack 4 `close` method
Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run. Fallback webpack 4 `close` method
@vercel
Copy link

vercel bot commented Aug 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nrwl/nx-dev/2xEjgV4hruiRSZPLXdYYG5jd7gCj
✅ Preview: Canceled

[Deployment for f2f6832 canceled]

@kazamov kazamov changed the title Fix close webpack compiler when build complete fix(core): close webpack compiler when build complete Aug 5, 2021
Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run. Fallback webpack 4 `close` method
@kazamov
Copy link
Contributor Author

kazamov commented Aug 6, 2021

@FrozenPandaz please take a look when you have time. I think it is an important change.

FrozenPandaz
FrozenPandaz previously approved these changes Aug 11, 2021
@FrozenPandaz FrozenPandaz dismissed their stale review August 11, 2021 18:26

Didn't see failing build

Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

It looks like this causes issues with other plugins. Can you take a look please?

@@ -26,7 +26,10 @@ export function runWebpack(config: any, webpack: any): Observable<any> {
} else {
webpackCompiler.run((err, stats) => {
callback(err, stats);
subscriber.complete();

webpackCompiler.close(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of monkey patching webpack above, perhaps it is easier to check for the existence of a close method?

if (webpackCompiler.close) {
  webpackCompiler.close(() => {
    subscriber.complete();
  });
} else {
  subscriber.complete();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FrozenPandaz, but if you check the close method in that case you will have a garbage after release of the Nx 13. If I do not mistake you will remove files with monkey patched code anyway. Thus it looks better in my opinion. Anyway it is up to you

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for thinking ahead!

That is okay because it is only 1 place. Please add a TODO comment saying to clean it up the conditional after Nx 13 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FrozenPandaz, done. I removed the fallback for webpack 4 and added if statement with TODO.

Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run.
Copy link
Collaborator

@FrozenPandaz FrozenPandaz left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you making those changes! LGTM 🎉

@FrozenPandaz FrozenPandaz merged commit 9966443 into nrwl:master Aug 13, 2021
FrozenPandaz pushed a commit that referenced this pull request Aug 17, 2021
* fix(core): close webpack compiler when build complete

Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run. Fallback webpack 4 `close` method
ManojBahuguna pushed a commit to ManojBahuguna/nx that referenced this pull request Sep 16, 2021
* fix(core): close webpack compiler when build complete

Call webpack `close` method when build complete according to https://webpack.js.org/api/node/#run. Fallback webpack 4 `close` method
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants