feat(migrate): CommonChunksPlugin to SplitChunksPlugin #558
Conversation
dhruvdutt
force-pushed
the
migrate
branch
3 times, most recently
from
a8ebc1d to
e4fb993
Compare
dhruvdutt
changed the title
There was a problem hiding this comment.
Left some comments and I think @sokra should be looking at the end output here too
| pluginProps.forEach((p: INode) => { | ||
| switch (p.key.name) { | ||
| case "names": | ||
| p.value.elements.forEach((chunkName) => { |
| ]; | ||
| switch (chunkName.value) { | ||
| case "vendor": | ||
| return j.property( |
There was a problem hiding this comment.
this is not always the case, user supplies the regex
There was a problem hiding this comment.
Can you please give an example syntax of how the user supplies the regex? I can't find it in the CommonsChunkPlugin docs
There was a problem hiding this comment.
If user supplies the test prop they can regex chunks
I can't find that option in the older CommonsChunkPlugin docs. test is supported in SplitChunksPlugin docs.
There was a problem hiding this comment.
No need to take review comments for actual comments. A regex to match a chunk was possible in v.2 if I recall right. @sokra needs to guide you here, I don't fully know backwards compat spec
|
|
||
| // create chunk cache group option | ||
| function createChunkCache(chunkName) { | ||
| const commonProperties = [ |
| @@ -0,0 +1,3 @@ | |||
| [*] | |||
|
If user supplies the test prop they can regex chunks |
dhruvdutt
force-pushed
the
migrate
branch
2 times, most recently
from
ebf2a98 to
846cc5a
Compare
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
| minChunks: 1, | ||
| test: 'vendor' | ||
| } | ||
| } |
| name: 'commons', | ||
| chunks: 'initial', | ||
| enforce: true, | ||
| minChunks: 1, |
There was a problem hiding this comment.
minChunks should be the number of defined entrypoints.
| optimizations: { | ||
| splitChunks: { | ||
| chunks: 'async', | ||
| minSize: 2000, |
There was a problem hiding this comment.
I would move this into the cacheGroups.main
There was a problem hiding this comment.
Still not resolved.
This case is not 100% map-able to splitChunks. This might be the best option:
cacheGroups: {
main: {
minSize: 2000,
chunks: 'async'
}
}| enforce: true, | ||
| minChunks: 1, | ||
| test: 'main' | ||
| } |
| enforce: true, | ||
| minChunks: 1, | ||
| test: 'main', | ||
| test: ({ resource }) => /node_modules/.test(resource) |
There was a problem hiding this comment.
You can't have test multiple times in the config.
You could be merged this way:
test: module => {
if(module.getChunks().some(chunk => chunk.name === 'main')) return true;
const fn = ({ resource }) => /node_modules/.test(resource);
return fn(module);
}| minChunks: 1, | ||
| test: 'main', | ||
|
|
||
| test: ({ resource }) => { |
| minChunks: 1, | ||
| test: 'main', | ||
|
|
||
| test: function ({ resource }) { |
| minChunks: 1, | ||
| test: 'main', | ||
|
|
||
| test: function ({ resource }) { |
webpack-bot
added
PR: reviewed-changes-requested
and removed
PR: review-outdated
labels
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
| chunks: 'async', | ||
|
|
||
| cacheGroups: { | ||
| minSize: 2000, |
There was a problem hiding this comment.
This is incorrect and will fail the validation.
|
|
||
| const fn = ({ resource }) => /node_modules/.test(resource); | ||
| return fn(module); | ||
| } |
| }; | ||
|
|
||
| return fn(module); | ||
| } |
|
|
||
| return fn(module); | ||
| } | ||
| } |
| }; | ||
|
|
||
| return fn(module); | ||
| } |
| optimizations: { | ||
| splitChunks: { | ||
| cacheGroups: { | ||
| minSize: 3000, |
| name: 'main', | ||
| chunks: 'initial', | ||
| enforce: true, | ||
| minChunks: 1, |
There was a problem hiding this comment.
You don't need minChunks: 1 when using enforce: true
| minChunks: 2, | ||
| test: 'vendor' | ||
| } | ||
| } |
There was a problem hiding this comment.
In this case runtimeChunk: { name: "vendor" } makes sense.
There was a problem hiding this comment.
The input webpack config doesn't have runtime anywhere. How to identify when to apply runtimeChunk?
This is the input case for this output:
module.exports = {
entry: {
app: './src/app.js',
vendor: './src/vendors.js',
},
plugins: [
new webpack.optimize.CommonsChunkPlugin({
names: ["app", "vendor"],
minChunks: 2
})
]
}| name: 'vendor', | ||
| chunks: 'initial', | ||
| enforce: true, | ||
| minChunks: 2, |
There was a problem hiding this comment.
minChunks: 2 is incorrect here.
When using test: "string", minChunks should not be set.
webpack-bot
added
PR: reviewed-changes-requested
and removed
PR: review-outdated
labels
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
| optimizations: { | ||
| splitChunks: { | ||
| chunks: 'async', | ||
| minSize: 2000, |
There was a problem hiding this comment.
Still not resolved.
This case is not 100% map-able to splitChunks. This might be the best option:
cacheGroups: {
main: {
minSize: 2000,
chunks: 'async'
}
}|
|
||
| optimizations: { | ||
| splitChunks: { | ||
| minSize: 3000, |
There was a problem hiding this comment.
This need to be moved into cacheGroups.main
dhruvdutt
changed the title
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
No
Summary
Initial implementation for migration CCP to SCP.
It is not possible to map all the options/properties from CCP to SCP migration. It's best to do a basic migration
optimization.splitChunksobject migration and give a link to docs, migration guide and allow the user the flexibility to make more adjustments/optimizations with newer options available in SCP. Also, many common use-cases directly work out of the box in the new SCP plugin.TODO:
Does this PR introduce a breaking change?
No
Other information
Closes #393