Skip to content

Added deepChildren support from ArcEglos' pull request #5764

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

Merged
merged 4 commits into from
Oct 11, 2017

Conversation

ljcrapo
Copy link
Member

@ljcrapo ljcrapo commented Oct 5, 2017

What kind of change does this PR introduce?

Feature

Did you add tests for your changes?

Yes

If relevant, link to documentation update:

webpack/webpack.js.org#1625

Summary

If you use the CommonsChunkPlugin with the children flag enabled it currently only cares about direct children. This change makes it so it cares about all children that are not required from somewhere else than the common chunk.
#3981

Does this PR introduce a breaking change?

If the config deepChildren isn't set the behavior stays the same as before, it only changes if the flag gets set.

Other information

Thanks to Florian Scholz (ArcEglos) for doing all the work.
#4255

@jsf-clabot
Copy link

jsf-clabot commented Oct 5, 2017

CLA assistant check
All committers have signed the CLA.

@ljcrapo
Copy link
Member Author

ljcrapo commented Oct 5, 2017

I see that the new test is failing in node v4. I'm working to resolve the issue.

destructuring not supported in Node v4
@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@wbern
Copy link

wbern commented Oct 6, 2017

Sweet! I am so waiting for this feature.

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Also add an test case in test/configCases or test/statsCases


return true;
});
let affectedChunks = [];
Copy link
Member

Choose a reason for hiding this comment

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

Make it a Set

## Uncompressed

```
Hash: fd4796594a8274a0759b
Copy link
Member

Choose a reason for hiding this comment

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

This should not include the color special chars. Try to pass --no-colors in examples/build-common.js to fix it.

deepChildren: true,
})
]
};
Copy link
Member

Choose a reason for hiding this comment

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

Could you also a async mode to this example? You can pass multiple configurations to include both modes in a single example.

 * using set instead of array
 * example of async use case
@webpack-bot
Copy link
Contributor

@ljcrapo Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Code looks good, example need to be fixed.

imports[1].default.should.be.eql("reuse");
done();
}).catch(e => {
done();
Copy link
Member

Choose a reason for hiding this comment

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

Pass the error to the done callback, elsewise errors are ignored.

@@ -0,0 +1 @@
require("../build-common");
Copy link
Member

Choose a reason for hiding this comment

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

add global.NO_TARGET_ARGS = true; before this line to disable the default and allow multiple configurations.

 * fixed README.md
 * pass error to done in test
@ljcrapo
Copy link
Member Author

ljcrapo commented Oct 10, 2017

@sokra or @timse what more needs to be done to get this merged in?

@ljcrapo
Copy link
Member Author

ljcrapo commented Oct 10, 2017

I added a pull request to the documentation for this feature.

@sokra sokra merged commit d9accb4 into webpack:master Oct 11, 2017
@sokra
Copy link
Member

sokra commented Oct 11, 2017

Thanks

@ZuBB
Copy link

ZuBB commented Oct 11, 2017

@sokra are you going to cook one more minor release before webpack 4?

@ljcrapo ljcrapo deleted the commons-chunk-deep-children branch October 11, 2017 17:08
@wbern
Copy link

wbern commented Oct 26, 2017

Can someone please look at #4392, specifically my question.

#4392 (comment)

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

Successfully merging this pull request may close these issues.

None yet

6 participants