-
-
Notifications
You must be signed in to change notification settings - Fork 622
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: Only try to remove deprecated plugins from array expressions #91
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
// This should throw | ||
export default (config) => { | ||
config.plugins.push(new webpack.optimize.UglifyJsPlugin()); | ||
config.plugins.push(new webpack.optimize.DedupePlugin()); | ||
config.plugins.push(new webpack.optimize.OccurrenceOrderPlugin()); | ||
return config | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,34 @@ | ||
const findPluginsByName = require('../utils').findPluginsByName; | ||
const codeFrame = require('babel-code-frame'); | ||
const utils = require('../utils'); | ||
|
||
module.exports = function(j, ast) { | ||
module.exports = function(j, ast, source) { | ||
// List of deprecated plugins to remove | ||
// each item refers to webpack.optimize.[NAME] construct | ||
const deprecatedPlugingsList = [ | ||
'webpack.optimize.OccurrenceOrderPlugin', | ||
'webpack.optimize.DedupePlugin' | ||
]; | ||
|
||
return findPluginsByName(j, ast, deprecatedPlugingsList) | ||
return utils.findPluginsByName(j, ast, deprecatedPlugingsList) | ||
.forEach(path => { | ||
// Check how many plugins are defined and | ||
// if there is only last plugin left remove `plugins: []` completely | ||
if (path.parent.value.elements.length === 1) { | ||
j(path.parent.parent).remove(); | ||
// For now we only support the case there plugins are defined in an Array | ||
if (path.parent && | ||
path.parent.value && | ||
utils.isType(path.parent.value, 'ArrayExpression') | ||
) { | ||
// Check how many plugins are defined and | ||
// if there is only last plugin left remove `plugins: []` node | ||
if(path.parent.value.elements.length === 1) { | ||
j(path.parent.parent).remove(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a valid way of removing ast nodes? I did not know at all. I usually come down from the parent and then modify it to remove any node. :( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what you mean by "valid way" but this works exactly as if I would come from the top of the tree. |
||
} else { | ||
j(path).remove(); | ||
} | ||
} else { | ||
j(path).remove(); | ||
const startLoc = path.value.loc.start; | ||
console.error(`For now only plugins instantiated in an array can be removed. | ||
${ codeFrame(source, startLoc.line, startLoc.column, { highlightCode: true }) } | ||
Please remove deprecated plugins manually. | ||
See https://webpack.js.org/guides/migrating/ for more information.`); | ||
} | ||
}); | ||
}; |
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.
What is the reason for adding
source
as an option?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.
In order to print the code block: https://github.com/webpack/webpack-cli/pull/91/files#diff-33fa12ea113366fa6b41cce3aeb9bf6dR29
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.
Oh, didn't notice!