-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Update js dependencies #2428
Update js dependencies #2428
Conversation
cbc2d8e
to
032d937
Compare
Just a few questions. Why is terser under resolutions? Should we avoid wrapping breaking changes in a commit like "Update js dependencies". Remember that users can fine-tune the loaders in |
Webpacker use And webpack has this dependency: a separate dependency was introduced in this commit 0d36af9#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L42 and then updated in this commit 13756f1#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L48 as a result, we have two terser-webpack-plugin: 2.3.1 I think terser-webpack-plugin will not be updated for webpack 4, because:
Webpack 4 supporting I use resolutions for |
yea, but webpacker in default configuration does not use those parameters that were changed. I think with the version update for Node.js, because
https://blog.risingstack.com/update-nodejs-8-end-of-life-no-support/ and also with this PR: #2415 We can also update and loaders |
package.json
Outdated
"jest": "^24.9.0" | ||
}, | ||
"resolutions": { | ||
"terser-webpack-plugin": "^2.3.1" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a result, we have two terser-webpack-plugin ... I think terser-webpack-plugin will not be updated for webpack 4
It is ok that we have 2 versions of terser. Webpack needs to use the version of terser that it was built to support. There is no need to micromanage the lock file 👍.
package.json
Outdated
@@ -43,22 +43,25 @@ | |||
"postcss-preset-env": "^6.7.0", | |||
"postcss-safe-parser": "^4.0.1", | |||
"regenerator-runtime": "^0.13.3", | |||
"sass-loader": "7.3.1", | |||
"style-loader": "^1.0.0", | |||
"sass-loader": "^8.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, but webpacker in default configuration does not use those parameters that were changed...We can also update and loaders
This will break things for users that chose to use those parameters since we can only update loaders in our own codebase. We could have changelog entry that informs users how to migrate their environment.js
, but this would need to be a breaking change.
@@ -8,31 +8,31 @@ | |||
"lib/install/config/webpacker.yml" | |||
], | |||
"engines": { | |||
"node": ">=8.16.0", | |||
"node": ">=10.13.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with the version update for Node.js because...
👍 I agree with this part
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything that I can help out to push this through the pipeline?
package.json
Outdated
"terser-webpack-plugin": "^2.3.4", | ||
"webpack": "^4.41.3", | ||
"webpack": "^4.41.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gauravtiwari, does it make sense to support this PR? Maybe it would be better to break it into smaller ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pustovalov Yep it does. I was just concerned if there is going to be any breaking changes? I guess everything looks good except file loader node version requirements? https://github.com/webpack-contrib/file-loader/releases/tag/v5.0.0
Thanks for working on this and sorry about the delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, looks like breaking change only in file-loader
If you could double check this and update. I will merge and make a release later today 👍 Thank you all 🙏 |
thanks @pustovalov |
This PR bumped
This broke asset loading mentioned in #2613 |
https://nodejs.org/en/about/releases/
https://github.com/webpack-contrib/file-loader/blob/master/CHANGELOG.md