Feature: Jscodeshift Transformations for --migrate #40

okonet · 2017-01-26T11:46:00Z

What kind of change does this PR introduce?

feature

Summary

This is where the work on #6 is happening.

TODOS

Migrations

Does this PR introduce a breaking change?

It should not

Other information

Issues:

If no value is set in query string it should convert it to true
Convert String to Number
Support Arrays in query string
Make sure special cases like Webpack 2? webpack-contrib/imports-loader#38 work
Support identifiers for object keys. For now it always creates literals for keys. This is tricky. See https://mathiasbynens.be/notes/javascript-properties [Lets choose literals only for now]

evenstensberg · 2017-01-26T12:14:46Z

I'm assigning @pksjce for all related work on transformations. Good work! 🎉

okonet · 2017-01-26T12:23:49Z

@ev1stensberg I'm a bit confused. Should I continue working on this?

evenstensberg · 2017-01-26T12:57:51Z

Oh yeah, I'm just making Pavithra head of reviewing and getting into the PR, evidently you should ask her for calibration on separation of work when she comes back

okonet · 2017-01-27T08:28:58Z

During my work on this PR I run into some nasty issues with tabs and had to replace them with spaces for fixtures. I saw we have been using eslint --fix to re-format the result of the transformation in order to eliminate those inconsistencies. The problem with this is that:

It basically leaves our code untested since we compare our transformations + eslint transformation with the input
For some reason it completely strip the result of the removeJsonLoaderTransform and I spent a few hours debugging it :(
We should probably allow re-formating the code with eslint but after all of the transforms are done (and not only in test runner)
This required some hard-code of for the jest test runner that is bundled with jscodeshift. This will make upgrading to the next version of jscodeshift very error prone if they decide to change the API internally.

I think it's much better to not rely on eslint formatting for our own tests. In order to be able to pass the ast down to transforms, I'd introduce a wrapper function that would take care of this. This would allow us to still use pre-bundled defineTest from jscodeshift and have modular transformations.

I’m also seeing some of the inconsistencies in the source code itself (as well as mixed usages of tabs and spaces). Should we switch to spaces to ensure we have fewer conflicts (I guess different editors and tools treat tabs differently) and enforce it with eslint? Now that we have auto-formatting in pre-commit hook with lint-staged, it will even fix these automatically.

Looks like webpack repo itself is using tabs, so we should probably stick with it: https://github.com/webpack/webpack/blob/master/.editorconfig#L4 I'd still enforce it with eslint.

Thoughts?

DanielaValero · 2017-01-27T10:52:48Z

Hi @okonet

About the tabs and spaces, I am with you on staying with tabs, so we are coherent with webpack.

I am not the best expert in code transformations, but anyways will check the branch and the work done on this matter so far, so I can soon enough start also coding :)

okonet · 2017-01-27T16:15:07Z

Another question is if we should check devtool option before applying this transform: https://webpack.js.org/guides/migrating/#uglifyjsplugin-sourcemap

pksjce · 2017-01-30T05:23:04Z

@okonet - Thanks a lot for this! Was looking for a review to validate my approach to this. Looking forward to working with you. Will be adding my comments on this by tomorrow.

pksjce · 2017-01-31T18:18:52Z

lib/transformations/defineTest.js

-const cli_engine = require('eslint').CLIEngine;
-const eslintrules = require(process.cwd() + '/.eslintrc.json');
-eslintrules.fix = true;
-const cli = new cli_engine(eslintrules);


I did this to counteract this issue with recast. But we are using a branch of recast right now, until this issue is fixed. So thanks for cleaning this up.

pksjce

Once you fix the uglifyJsPlugin file reference, this is ready to merge

pksjce · 2017-01-31T18:20:25Z

lib/transformations/index.js

@@ -1,7 +1,11 @@
 const loaderTransform = require('./loaders/loaders');
 const resolveTransform = require('./resolve/resolve');
+const removeJsonLoaderTransform = require('./removeJsonLoader/removeJsonLoader');
+const uglifyJsPluginTransform = require('./uglifyJsPluginTransform/uglifyJsPluginTransform');


This file is uglifyJsPlugin

What do you mean? I'd add Transform suffix to all transforms just to make it more explicit. Thoughts?

pksjce · 2017-01-31T18:24:40Z

lib/transformations/loaders/loaders.js

-										j.literal(val)
-										)]));
-		var loaderArray = j.arrayExpression(objs);
+		let objs = p.parent.node.value.value.split('!')


Note - Let's move such references(which are necessary) to some common meaningful methods.

I'm not sure it will work in any context but I agree we should try reuse as much as possible. I'd wait till we see more repetitions before we start adding abstractions.

pksjce · 2017-01-31T18:25:38Z

lib/transformations/loaders/loaders.js

-		if(val === 'true') literalVal = true;
-		if(val === 'false') literalVal = false;
+		let literalVal = val;
+		// We'll need String to native type conversions


How to do this? Is there some generic way to convert?

Well, this is what it methods implements, basically. ↓

pksjce · 2017-01-31T18:27:46Z

lib/transformations/occurenceOrderPlugin/__testfixtures__/.editorconfig

@@ -0,0 +1,3 @@
+[*]
+indent_style = space
+indent_size = 4


Does this need to be committed?

It does if you use editoconfig plugin in your IDE. Why not include it?

pksjce · 2017-01-31T18:28:27Z

lib/transformations/occurenceOrderPlugin/__testfixtures__/occurenceOrderPlugin.output.js

@@ -0,0 +1,7 @@
+module.exports = {
+    plugins: []


Better to remove the plugins param if the array is empty?

I was thinking about it but still not sure about it. Do you think it won't be too obscure to just remove it from the config?

pksjce · 2017-01-31T18:35:34Z

Also, the no-var lint rule is breaking the build. Maybe turn it into a warning and slowly move up to error?

okonet · 2017-02-01T11:38:15Z

.eslintrc.json

@@ -29,6 +29,7 @@
            "always"
        ],
        "no-unused-vars": 1,
-        "no-console": 0
+        "no-console": 0,
+        "no-var": "error"


@pksjce I've added this rule to enable eslint --fix for it but I agree this should be in a separate PR at least. I'll revert the commit for now and rebase the branch.

evenstensberg · 2017-02-13T06:51:48Z

@pksjce You've got conflicts

okonet · 2017-02-13T09:59:59Z

I've fixed all confilcts 😎

okonet · 2017-02-15T17:19:21Z

Some updates on this PR:

Lots of changes in the folder structure. We could simplify even more since there are still lots of bootstrapping code in there but I wanted to stop somewhere :)
Switched to Jest Snapshots to simplify test cases creation
Exporting transform function that takes source and list of transformations as arguments. This one is re-used in the migrate command

Help needed:

How to remove the right debug: true? Other plugins could use same key:value.

webpack-cli/lib/transformations/loaderOptionsPlugin/loaderOptionsPlugin.js

Line 9 in 2e4e011

// TODO: I can't figure out how to find the topmost `debug: true`. help!
It would be great to validate config after transformation. Is there a package for that?
We're still having issues with recast. https://travis-ci.org/webpack/webpack-cli/jobs/201945033#L592 See Recast is adding extra newlines in ObjectExpressions benjamn/recast#242

pksjce · 2017-02-15T17:59:57Z

For Validation you should use https://github.com/webpack/webpack/blob/master/lib/webpack.js#L58

evenstensberg · 2017-02-15T18:01:43Z

@okonet @pksjce In the parser branch, the validation logic is included. Should wait until that is merged perhaps? I'm aiming to get it ready before monday.

okonet · 2017-02-15T19:50:21Z

I'm fine with merging this as soon as we get all transforms ready. We can continue in separate PRs after that.

pksjce · 2017-02-19T12:50:57Z

lib/transformations/utils.js

+ * @param { Node } node - Node to start search from
+ * @returns Path
+ * */
+function findPluginsRootNodes(j, node) {


Hey why can't this be like findAnyRootNode(j, node, nodeName){}

It can totally be if you need it to!

pksjce · 2017-02-19T15:03:04Z

lib/transformations/utils.js

+function createProperty(j, key, value) {
+	return j.property(
+		'init',
+		j.literal(key),


This has to be j.identifier(key). Will change this and also related snapshots.

pksjce · 2017-02-19T15:05:25Z

lib/transformations/utils.test.js

+			expect(j(j.objectExpression([res])).toSource()).toMatchSnapshot();
+		});
+
+		it('should create properties for non-literal keys', () => {


Do we ever create properties with non-literal keys?

Don't think so but we should be aware of the issue.

okonet · 2017-02-19T15:13:35Z

lib/transformations/utils.js

@@ -83,7 +83,7 @@ function findPluginsRootNodes(j, node) {
 function createProperty(j, key, value) {
 	return j.property(
 		'init',
-		j.literal(key),
+		j.identifier(key),


This won't work with anything more complex like foo-bar but I think it's okay in the scope of this project.

Yeah, so key in a Property can be Identifier or Literal, so for safety we'll keep it Literal?

Yeah but as far as I can see we don't use anything but identifiers in our code so I'd create a separate GHI, replace it to j.identifier and add a comment to the GHI for now. Since this style will break linting of lots of code I guess.

okonet · 2017-02-19T15:14:13Z

lib/transformations/__snapshots__/utils.test.js.snap

+  foo: true,
+  bar: \"baz\",
+  foo: false,
+  baz-long: true


Here: it's not a valid JS anymore

okonet · 2017-02-19T15:16:36Z

lib/transformations/utils.test.js

@@ -23,7 +23,7 @@ describe('utils', () => {
 			expect(j(j.objectExpression([res])).toSource()).toMatchSnapshot();
 		});

-		it('should create properties for non-literal keys', () => {
+		xit('should create properties for non-literal keys', () => {


Is this a typo?

pksjce · 2017-02-19T15:56:22Z

Hey! @okonet - I have passed the tests and checked off ExtractPlugin changes. I will add more special cases in smaller PRs later?

For "Support Arrays in query string" can you give an example?
For "Make sure special cases like Webpack 2? webpack-contrib/imports-loader#38 work", shall we take it up in another PR?

This needs more work but ready to merge for now.

okonet · 2017-02-19T18:41:35Z

Yes, I think the opened issues should be separate GHIs and PRs accordingly. Let's make this mergable by rebasing on master and we're good to go.

It takes source and array of transformations and returns the new source code. Added "integration" tests.

dentuzhik · 2017-05-02T16:13:20Z

I know this is a stupid for this PR, but my searches resulted in pretty much nothing.
Is there any way to use this already, or this is still a WIP?

I tried to clone and run it on my configs which are constructed using webpack-config, did not work out nicely :)

I have nothing against manual upgrade, but having an automatic version is always better :)

evenstensberg · 2017-05-02T16:45:49Z

Yeah, you can use npm link, then webpack-cli --migrate myconfig

okonet · 2017-05-02T20:59:50Z

@dentuzhik I'm not sure it will work with webpack-config. You can report issues with input and output but I'm not sure we'll support non-standard syntaxes in the beginning.

missing1984 · 2017-05-03T18:43:34Z

This is great, is there any way that i can provide a webpack1 config object instead of a file?

okonet · 2017-05-04T13:37:21Z

@missing1984 what do you mean by that? stdin? Or what do you have in mind? Also, not at the moment and I'm not sure what's the use case.

missing1984 · 2017-05-04T18:21:55Z

@okonet In our use case, we break the webpack config into many small pieces and merge(webpack-merge) or layer them together to serve different deployment needs. Is the migration cli going to help?

dentuzhik · 2017-05-06T09:58:56Z

@missing1984 I achieve the same thing via webpack-config package, I linked above and seems that in this case automatic migration will not be entirely possible.

okonet · 2017-05-08T09:59:29Z

I think it will work if you operate on objects. The best way to know would be to try running it on your setup and let us know if something isn't working.

thom801 · 2017-06-23T21:59:29Z

@pksjce and @okonet you guys are amazing thank you for working so hard and creating this.

arthurgeron · 2017-10-21T01:43:00Z

I'm a little lost, I've tried using webpack-cli --migrate webpack.config.babel.js but it says migrate is an unknown argument.

evenstensberg · 2017-10-21T15:37:43Z

Try without the dashes

okonet mentioned this pull request Jan 26, 2017

Feature [--migrate]: Migrate from webpack 1 to 2 through webpack-cli #6

Closed

12 tasks

evenstensberg assigned pksjce Jan 26, 2017

evenstensberg added AST Transform v1 labels Jan 26, 2017

okonet self-assigned this Jan 27, 2017

okonet requested a review from pksjce January 27, 2017 08:20

pksjce reviewed Jan 31, 2017

View reviewed changes

pksjce mentioned this pull request Jan 31, 2017

Scoping: migrate feature #33

Closed

1 task

okonet commented Feb 1, 2017

View reviewed changes

pksjce reviewed Feb 19, 2017

View reviewed changes

okonet commented Feb 19, 2017

View reviewed changes

okonet and others added 12 commits February 20, 2017 02:28

Simplified directory structure and colocate tests with source files.

83c65db

Switch to snapshot testing and removed output fixtures

c12df12

All transformations return AST to allow chaining

45ccf97

Split separate test cases into separate files

99624b5

Added tests and fixed some edge cases in BannerPlugin

6f04f3d

Introduce and export transform function

4223c3a

It takes source and array of transformations and returns the new source code. Added "integration" tests.

Adds new utils and tests

067f71a

Refactors loaders transforms

99e99cd

Adds more refactoring

3e064b4

Trims the input test

d758330

Fixes eslint and parse bug

d66f4e1

Adds to the contributor docs about transforms

e0539a1

pksjce force-pushed the feature/jscodeshift-transforms branch from 60d8789 to e0539a1 Compare February 19, 2017 20:58

pksjce changed the title ~~WIP: JS Codeshift Transformations~~ Feature: Jscodeshift Transformation for --migrate Feb 19, 2017

pksjce changed the title ~~Feature: Jscodeshift Transformation for --migrate~~ Feature: Jscodeshift Transformations for --migrate Feb 19, 2017

pksjce merged commit f0abf0c into master Feb 19, 2017

pksjce deleted the feature/jscodeshift-transforms branch February 20, 2017 10:19

pksjce mentioned this pull request Feb 21, 2017

Feature [--migrate]: Change migrate script tests to have jest snapshots #50

Closed

evenstensberg mentioned this pull request May 6, 2017

Feature: Migrate current webpack cli from webpack to webpack-cli #30

Closed

Feature: Jscodeshift Transformations for --migrate #40

Feature: Jscodeshift Transformations for --migrate #40

Conversation

okonet commented Jan 26, 2017 • edited Loading

TODOS

Migrations

evenstensberg commented Jan 26, 2017

okonet commented Jan 26, 2017

evenstensberg commented Jan 26, 2017

okonet commented Jan 27, 2017 • edited Loading

DanielaValero commented Jan 27, 2017

okonet commented Jan 27, 2017

pksjce commented Jan 30, 2017

Choose a reason for hiding this comment

pksjce left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksjce commented Jan 31, 2017

Choose a reason for hiding this comment

evenstensberg commented Feb 13, 2017

okonet commented Feb 13, 2017 • edited Loading

okonet commented Feb 15, 2017 • edited by pksjce Loading

Help needed:

pksjce commented Feb 15, 2017

evenstensberg commented Feb 15, 2017

okonet commented Feb 15, 2017

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

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

pksjce commented Feb 19, 2017

okonet commented Feb 19, 2017

dentuzhik commented May 2, 2017 • edited Loading

evenstensberg commented May 2, 2017

okonet commented May 2, 2017

missing1984 commented May 3, 2017

okonet commented May 4, 2017

missing1984 commented May 4, 2017

dentuzhik commented May 6, 2017

okonet commented May 8, 2017

thom801 commented Jun 23, 2017

arthurgeron commented Oct 21, 2017

evenstensberg commented Oct 21, 2017

okonet commented Jan 26, 2017 •

edited

Loading

okonet commented Jan 27, 2017 •

edited

Loading

okonet commented Feb 13, 2017 •

edited

Loading

okonet commented Feb 15, 2017 •

edited by pksjce

Loading

dentuzhik commented May 2, 2017 •

edited

Loading