Update extractTextPlugin transformation #390

bitpshr · 2018-04-09T14:32:35Z

What kind of change does this PR introduce?
Feature

Did you add tests for your changes?
Yes

If relevant, did you update the documentation?
N/A

Summary
After some discussion the decision was made to update our migration transformations to actively support migrating only from the previous version to the current version, which currently means v3 - v4. Migrating older versions such as v1 or v2 will no longer be supported. This PR begins this work by updating the extractTextPlugin transformation. Resolves #373.

Note: v3 configurations that contain multiple ExtractTextPlugin instances are not yet supported due to a limitation with MiniCssExtractPlugin.

Does this PR introduce a breaking change?
Yes. This PR changes ExtractTextPlugin migration behavior and produces a v4-specific configuration.

Other information
This PR only includes updates to the extractTextPlugin transformation. The following issues have been opened for the remaining transformations:

bitpshr · 2018-04-09T14:37:18Z

lib/migrate/extractTextPlugin/__testfixtures__/extractTextPlugin.input.js

-                use: ExtractTextPlugin.extract('style-loader', 'css-loader')
+                use: ExtractTextPlugin.extract({
+                    use: 'css-loader',
+                    fallback: ['style-loader', { loader: 'test-object-loader' }],


I tried to fully demonstrate that string, object, and array loader configurations all work via this test case.

bitpshr · 2018-04-09T14:43:30Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+	// Add new loader
+	loaderPath && Object.assign(loaderPath, {
+		value: currentFallbackLoader ?
+			j.conditionalExpression(


MiniCssExtractPlugin does not support fallback and disabled options. Instead, users should manually swap out MiniCssExtractPlugin.loader with the loader that was normally used as a fallback, which is what we're doing with this ternary. More information can be found here.

matheus1lva · 2018-04-09T14:49:14Z

I found some problems. I have this configuration (that was not created by me) and i think it is a good "point out of the curve", since people can still have things like this: https://gist.github.com/PlayMa256/0a51b41e1b0eba910869764f378bf932

This was the generated config: https://gist.github.com/PlayMa256/587b2d3890c9c96e3f3dbe567c8ecfce

As you can see, few problems occurred:

It has 2 empty variables, which were kept in the plugins list.
MiniCssPlugin has null parameters. I think it is better to remove them other then having them null.
Is there a way to remove the extractCSS.extract statements and replace with MiniCssExtractPlugin.loader?

bitpshr · 2018-04-09T14:52:04Z

Good catch and feedback. On it now.

matheus1lva · 2018-04-09T17:33:41Z

Well, the null problems seems to be gone. In the example i sent you (which is kind absurd i
admit), it still have (extractSCSS.extract...). We can't control that much, neither handle the problem in this case, where there are fallbacks and such.

Could you create a test for multiple loaders in a rule?
I mean:

[
          'style-loader',
          'css-loader',
          ExtractTextPlugin...
]

A scenario similar to this? That could show problems if it fails.

bitpshr · 2018-04-09T20:44:43Z

@playma256 @ematipico I updated the logic so options are only passed if they're defined and null is never passed. I also updated the tests to verify cases when ExtractTextPlugin.extract is an argument to a function call (like your example, ['css-hot-loader'].concat(ExtractTextPlugin.extract(...)), and cases when ExtractTextPlugin.extract is a member of an array (like your multiple loaders scenario above.) All are working correctly.

After digging into multiple-instance support a bit more, it looks like we need to only support migrating configurations with a single instance of ExtractTextPlugin until true multiple-instance support lands in MiniCssExtractPlugin. See webpack-contrib/mini-css-extract-plugin#45. Until that happens, it's not possible to translate per-instance options such as filename. We could technically translate extractCss.extract-type call statements to ExtractTextPlugin.extract statements, but it wouldn't accomplish much since MiniCssExtractPlugin only supports one definition in the plugins array for now.

This is the best we can do for now and we can put a caveat in the documentation until webpack-contrib/mini-css-extract-plugin#45 is resolved.

matheus1lva · 2018-04-09T20:49:33Z

Oh, got it! I did not know about this problem, i just found this tricky webpack config and it could be a nice way to debug this feature :). No problem not supporting this, we could insert this disclaimer in the docs. I'm about to go home, as i arrive and have time to test, i can give you more feedback if necessary! Good job @billyjanitsch !!

evenstensberg · 2018-04-11T17:46:27Z

@ematipico this is a nice PR for you to test & to get more knowledge of the migrate internals. Easy testable as well.

evenstensberg

Can you abstract this into JScodeshift utilities? A loot of specific code for the nodes, would be nice to use things like j(p.identifier) etc

bitpshr · 2018-04-13T18:31:24Z

Thanks, I moved most functionality to ast-utils and made it generic so other transformations can use it going forward. This should be ready for another review when you have time @ev1stensberg @ematipico.

evenstensberg · 2018-04-14T17:12:04Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

 *
 * @param {Object} j - jscodeshift top-level import
 * @param {Node} ast - jscodeshift ast to transform
 * @returns {Node} ast - jscodeshift ast
 */
-
 module.exports = function(j, ast) {


named functions

evenstensberg · 2018-04-14T17:12:34Z

lib/utils/ast-utils.js

+ * @param {any} j — jscodeshift API
+ * @param {Node} node - Node to start search from
+ * @param {String} pluginName - name of the plugin to remove
+ * @param {Node | Void} - path to the root webpack configuration object if plugin is found


Missing return

evenstensberg · 2018-04-14T17:12:49Z

lib/utils/ast-utils.js

+ * Calls an function with a path wrapped in an array, using path.elements if available
+ *
+ * @param {NodePath} path - path to wrap in array
+ * @param {Function} action - called with array path


Missing return

Added @returns {Void}.

evenstensberg · 2018-04-14T17:14:09Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

-				)
-			);
-			p.value.arguments = [newArgs];
+	const devLoaders = [];


Can you abstract all this into more functions that can be reused, like find value "filename" for a given prop

I'm not sure I follow; we already abstracted functionality into simple, single-purpose functions like getPluginOptionValue and getLoaderOptionValue. If we start getting even more abstract than this, we risk targeting nodes that aren't actually the nodes we care about. For example, someone could define filename completely outside of the webpack configuration object for unrelated purposes, so we need to be very careful which filename identifier we actually target.

The functions I already pulled out (createPluginByName, removePluginByName, getLoaderOptionValue, getPluginOptionValue, and removeRequire) are reusable and safe.

webpack-bot · 2018-04-14T17:27:56Z

@bitpshr Thanks for your update.

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

@ev1stensberg Please review the new changes.

evenstensberg · 2018-04-14T18:18:04Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+	let pluginExists;
+	let requireExists;
+
+	const filename = getPluginOptionValue(j, ast, "ExtractTextPlugin", "filename");


Could you extract these into more functions that could be reused?

evenstensberg · 2018-04-14T18:18:23Z

lib/utils/ast-utils.js

+ * @param {any} j — jscodeshift API
+ * @param {Node} node - Node to start search from
+ * @param {String} pluginName - ex. `webpack.LoaderOptionsPlugin`
+ * @param {Object} options - plugin options


evenstensberg · 2018-04-14T18:18:29Z

lib/utils/ast-utils.js

+ * @param {any} j — jscodeshift API
+ * @param {Node} node - Node to start search from
+ * @param {String} constName - Name of require
+ * @param {String} packagePath - path of required package


bitpshr · 2018-04-14T19:30:59Z

@ev1stensberg thanks for the reviews. Everything has been addressed except for the following comment:

Could you extract these into more functions that could be reused?

What type of functions were you imagining? I already abstracted almost all logic into ast-utils so that this transform is mostly one-liner calls into ast-utils. The functions I already added (createPluginByName, removePluginByName, getLoaderOptionValue, getPluginOptionValue, and removeRequire) are reusable and safe, and as abstract as any other function in ast-utils.

bitpshr · 2018-04-16T22:25:35Z

Thanks @ematipico. Could you please see my comment above and take another look when you have a chance @ev1stensberg? Thanks in advance!

evenstensberg · 2018-04-17T12:10:06Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+	pluginExists && createPluginByName(j, ast, "MiniCssExtractPlugin", loaderOptions);
+
+	// Remove old loader
+	ast.find(j.Identifier, { name: "ExtractTextPlugin" })


This could be abstracted into an util, maybe you can make it general so that we can reuse it for later?

I pulled most of this logic out into a new findLoaderFunction utility function. It allows us to target loaders that use call expressions, e.g. use: SomeIdentifier(...). I could not pull everything out into a removeLoaderFunction because we need information during the traversal. This is the best we can do.

evenstensberg · 2018-04-17T12:10:19Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+	});
+
+	// Add new loader
+	const loader = currentFallbackLoader ?


This might be another util, addLoader

evenstensberg · 2018-04-17T12:10:39Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+
+	// Add new loader
+	const loader = currentFallbackLoader ?
+		j.conditionalExpression(


It might be too specific, is there another way of adding a loader?

I created an addConditionalLoader utility function per your comment above. I think it'll be useful in the long run, as it makes it easy to add loaders based on NODE_ENV.

bitpshr · 2018-04-17T16:32:36Z

Thanks @ev1stensberg. This should be as generic as possible now. Let me know your thoughts.

webpack-bot · 2018-04-17T16:52:37Z

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

evenstensberg

LGTM, we can always change it later

bitpshr · 2018-04-18T12:38:03Z

Thanks @ev1stensberg. Rebased.

webpack-bot added PR: unreviewed PR: CI-not-ok labels Apr 9, 2018

bitpshr commented Apr 9, 2018

View reviewed changes

webpack-bot added PR: CI-ok and removed PR: CI-not-ok labels Apr 9, 2018

ematipico added the Semver: minor ⚙️ When delivering new features that don't break label Apr 9, 2018

This was referenced Apr 10, 2018

[Feature] Migrate should move ModuleConcatenationPlugin to optimizations.concatenateModules #396

Closed

[Feature] Migrate should move NoEmitOnErrorsPlugin to optimizations.noEmitOnErrors #395

Closed

ematipico self-requested a review April 12, 2018 12:01

evenstensberg reviewed Apr 13, 2018

View reviewed changes

webpack-bot added PR: CI-not-ok PR: CI-ok and removed PR: CI-ok PR: CI-not-ok labels Apr 13, 2018

evenstensberg requested changes Apr 14, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated and removed PR: unreviewed PR: reviewed-changes-requested labels Apr 14, 2018

evenstensberg reviewed Apr 14, 2018

View reviewed changes

ematipico approved these changes Apr 16, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Apr 16, 2018

evenstensberg requested changes Apr 17, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested PR: review-outdated and removed PR: reviewed-approved PR: reviewed-changes-requested labels Apr 17, 2018

webpack-bot added PR: CI-not-ok PR: CI-ok and removed PR: CI-ok PR: CI-not-ok labels Apr 17, 2018

evenstensberg approved these changes Apr 18, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Apr 18, 2018

bitpshr added 6 commits April 18, 2018 08:28

ast(migrate): update extractTextPlugin transformation v3 -> v4

6e58f6b

ast(migrate): update extractTextPlugin transformation rules

f7a8969

ast(migrate): add utilities to ast-utils

48f0ddf

ast(migrate): add more jsdoc for ast-utils

2f4d8ef

ast(migrate): add more jsdoc for ast-utils

6f3402d

ast(migrate): extract logic into ast-utils

9d04a70

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Apr 18, 2018

ematipico merged commit 936b80c into webpack:master Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update extractTextPlugin transformation #390

Update extractTextPlugin transformation #390

bitpshr commented Apr 9, 2018 •

edited

Loading

bitpshr Apr 9, 2018

bitpshr Apr 9, 2018 •

edited

Loading

matheus1lva commented Apr 9, 2018 •

edited

Loading

bitpshr commented Apr 9, 2018

matheus1lva commented Apr 9, 2018 •

edited

Loading

bitpshr commented Apr 9, 2018 •

edited

Loading

matheus1lva commented Apr 9, 2018

evenstensberg commented Apr 11, 2018

evenstensberg left a comment

bitpshr commented Apr 13, 2018 •

edited

Loading

evenstensberg Apr 14, 2018

evenstensberg Apr 14, 2018

evenstensberg Apr 14, 2018

bitpshr Apr 14, 2018 •

edited

Loading

evenstensberg Apr 14, 2018

bitpshr Apr 14, 2018 •

edited

Loading

webpack-bot commented Apr 14, 2018

evenstensberg Apr 14, 2018

evenstensberg Apr 14, 2018

evenstensberg Apr 14, 2018

bitpshr commented Apr 14, 2018 •

edited

Loading

bitpshr commented Apr 16, 2018

evenstensberg Apr 17, 2018

bitpshr Apr 17, 2018

evenstensberg Apr 17, 2018

evenstensberg Apr 17, 2018

bitpshr Apr 17, 2018

bitpshr commented Apr 17, 2018

webpack-bot commented Apr 17, 2018

evenstensberg left a comment

bitpshr commented Apr 18, 2018

Update extractTextPlugin transformation #390

Update extractTextPlugin transformation #390

Conversation

bitpshr commented Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

bitpshr Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

matheus1lva commented Apr 9, 2018 • edited Loading

bitpshr commented Apr 9, 2018

matheus1lva commented Apr 9, 2018 • edited Loading

bitpshr commented Apr 9, 2018 • edited Loading

matheus1lva commented Apr 9, 2018

evenstensberg commented Apr 11, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

bitpshr commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr Apr 14, 2018 • edited Loading

Choose a reason for hiding this comment

webpack-bot commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr commented Apr 14, 2018 • edited Loading

bitpshr commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitpshr commented Apr 17, 2018

webpack-bot commented Apr 17, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

bitpshr commented Apr 18, 2018

bitpshr commented Apr 9, 2018 •

edited

Loading

bitpshr Apr 9, 2018 •

edited

Loading

matheus1lva commented Apr 9, 2018 •

edited

Loading

matheus1lva commented Apr 9, 2018 •

edited

Loading

bitpshr commented Apr 9, 2018 •

edited

Loading

bitpshr commented Apr 13, 2018 •

edited

Loading

bitpshr Apr 14, 2018 •

edited

Loading

bitpshr Apr 14, 2018 •

edited

Loading

bitpshr commented Apr 14, 2018 •

edited

Loading