-
-
Notifications
You must be signed in to change notification settings - Fork 597
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
Feature: Jscodeshift Transformations for --migrate #40
Conversation
I'm assigning @pksjce for all related work on transformations. Good work! 🎉 |
@ev1stensberg I'm a bit confused. Should I continue working on this? |
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 |
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
I think it's much better to not rely on eslint formatting for our own tests. In order to be able to pass the I’m also seeing some of the inconsistencies in the source code itself (as well as mixed usages of tabs and spaces). 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? |
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 :) |
Another question is if we should check |
@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. |
lib/transformations/defineTest.js
Outdated
const cli_engine = require('eslint').CLIEngine; | ||
const eslintrules = require(process.cwd() + '/.eslintrc.json'); | ||
eslintrules.fix = true; | ||
const cli = new cli_engine(eslintrules); |
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 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.
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.
Once you fix the uglifyJsPlugin
file reference, this is ready to merge
lib/transformations/index.js
Outdated
@@ -1,7 +1,11 @@ | |||
const loaderTransform = require('./loaders/loaders'); | |||
const resolveTransform = require('./resolve/resolve'); | |||
const removeJsonLoaderTransform = require('./removeJsonLoader/removeJsonLoader'); | |||
const uglifyJsPluginTransform = require('./uglifyJsPluginTransform/uglifyJsPluginTransform'); |
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.
This file is 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.
What do you mean? I'd add Transform
suffix to all transforms just to make it more explicit. Thoughts?
j.literal(val) | ||
)])); | ||
var loaderArray = j.arrayExpression(objs); | ||
let objs = p.parent.node.value.value.split('!') |
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.
Note - Let's move such references(which are necessary) to some common meaningful methods.
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'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.
if(val === 'true') literalVal = true; | ||
if(val === 'false') literalVal = false; | ||
let literalVal = val; | ||
// We'll need String to native type conversions |
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.
How to do this? Is there some generic way to convert?
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.
Well, this is what it methods implements, basically. ↓
@@ -0,0 +1,3 @@ | |||
[*] | |||
indent_style = space | |||
indent_size = 4 |
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.
Does this need to be committed?
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 does if you use editoconfig plugin in your IDE. Why not include it?
@@ -0,0 +1,7 @@ | |||
module.exports = { | |||
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.
Better to remove the plugins param if the array is empty?
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 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?
Also, the |
.eslintrc.json
Outdated
@@ -29,6 +29,7 @@ | |||
"always" | |||
], | |||
"no-unused-vars": 1, | |||
"no-console": 0 | |||
"no-console": 0, | |||
"no-var": "error" |
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.
@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.
@pksjce You've got conflicts |
I've fixed all confilcts 😎 |
Some updates on this PR:
Help needed:
|
For Validation you should use https://github.com/webpack/webpack/blob/master/lib/webpack.js#L58 |
I'm fine with merging this as soon as we get all transforms ready. We can continue in separate PRs after that. |
* @param { Node } node - Node to start search from | ||
* @returns Path | ||
* */ | ||
function findPluginsRootNodes(j, node) { |
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 why can't this be like findAnyRootNode(j, node, nodeName){}
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 can totally be if you need it to!
lib/transformations/utils.js
Outdated
function createProperty(j, key, value) { | ||
return j.property( | ||
'init', | ||
j.literal(key), |
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.
This has to be j.identifier(key)
. Will change this and also related snapshots.
expect(j(j.objectExpression([res])).toSource()).toMatchSnapshot(); | ||
}); | ||
|
||
it('should create properties for non-literal keys', () => { |
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.
Do we ever create properties with non-literal keys?
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.
Don't think so but we should be aware of the issue.
lib/transformations/utils.js
Outdated
@@ -83,7 +83,7 @@ function findPluginsRootNodes(j, node) { | |||
function createProperty(j, key, value) { | |||
return j.property( | |||
'init', | |||
j.literal(key), | |||
j.identifier(key), |
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.
This won't work with anything more complex like foo-bar
but I think it's okay in the scope of this project.
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.
Yeah, so key in a Property can be Identifier or Literal, so for safety we'll keep it Literal?
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.
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.
foo: true, | ||
bar: \"baz\", | ||
foo: false, | ||
baz-long: true |
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.
Here: it's not a valid JS anymore
lib/transformations/utils.test.js
Outdated
@@ -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', () => { |
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 a typo?
Hey! @okonet - I have passed the tests and checked off
This needs more work but ready to merge for now. |
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.
60d8789
to
e0539a1
Compare
I know this is a stupid for this PR, but my searches resulted in pretty much nothing. 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 :) |
Yeah, you can use |
@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. |
This is great, is there any way that i can provide a webpack1 config object instead of a file? |
@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. |
@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? |
@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. |
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. |
I'm a little lost, I've tried using |
Try without the dashes |
What kind of change does this PR introduce?
feature
Summary
This is where the work on #6 is happening.
TODOS
Migrations
enforce: 'pre/post'
-loaders
suffix.json-loader
sourceMap: true
toUglifyJsPlugin
ExtractTextPlugin
syntaxOccurendeOrderPlugin
webpack.optimize.DedupePlugin
LoaderOptionsPlugin
withminimize:true
,andcontext
debug
Does this PR introduce a breaking change?
It should not
Other information
Issues:
true
String
toNumber