-
-
Notifications
You must be signed in to change notification settings - Fork 603
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 UglifyJSPlugin transformation #401
Update UglifyJSPlugin transformation #401
Conversation
@@ -2,36 +2,110 @@ | |||
|
|||
exports[`uglifyJsPlugin transforms correctly using "uglifyJsPlugin-0" data 1`] = ` | |||
"module.exports = { | |||
plugins: [ | |||
new webpack.optimize.UglifyJsPlugin({ |
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 found a problem here, something i left behind... i'm fixing it now.
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.
Done.
.filter(j.filters.VariableDeclarator.requiresModule("uglifyjs-webpack-plugin")); | ||
|
||
// Look for a variable declaration which requires uglifyjs-webpack-plugin | ||
// saves the name of this variable. |
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.
Multi line comment here
// options passed to plugin | ||
const pluginOptions = node.value.arguments; | ||
|
||
// check if there are any options passed to UglifyWebpackPlugin |
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.
ML comment.
// If user is using UglifyJsPlugin directly from webpack | ||
// transformation must: | ||
// - remove it | ||
// - add require for uglify-webpack-plugin |
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.
- .. -
sourceMap: true | ||
}) | ||
] | ||
plugins: [], |
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.
Why's this empty? Should be preserved when uglifyJSPlugin
is correct?
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 missed this cleanup. I'm going to create an utility to remove plugins property if they are empty, so it can be used by other transformations that fell into the same problem.
@playma256 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ev1stensberg Please review the new changes. |
I'm adding tests to this new method. I just messed up a bit, i'm fixing them. |
@@ -90,6 +90,22 @@ exports[`utils createProperty should create properties for non-literal keys 1`] | |||
}" | |||
`; | |||
|
|||
exports[`utils findPluginsArrayAndRemoveIfEmpty should not remove plugins property, since it has length > 0 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.
Use proper grammar here.
lib/utils/ast-utils.js
Outdated
* remove if it has no elements. | ||
* @param {any} j - jscodeshift API | ||
* @param {Node} rootNode - node to start search from | ||
*/ |
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.
Missing jsdoc for return here
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.
It lookouts for the plugins property and, the array is empty, it removes it from the AST
optimization: { | ||
minimize: true, | ||
|
||
minimizer: [new UglifyJsPlugin({ |
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 this correct? Could you link me to the docs on this?
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.
Oki, good stuff
All the small fixes which were required are done! |
Created function on ast-utils so every other transformation can use this.
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
I was looking at this, to tell my workmates that they could use to upgrade the webpack config on their projects, but i was not able to see my commits on the What happened (@ematipico @ev1stensberg )? |
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
Yes.
Tests included to test scenarios where:
webpack.optimize.UglifyJsPlugin
without custom optionswebpack.optimize.UglifyJsPlugin
with custom optionsuglify-webpack-plugin
with custom optionsuglify-webpack-plugin
without custom optionsSummary
Fix #389
Does this PR introduce a breaking change?
Yes. It deprecates migration from v1->v2 and only supports v3->v4