feat: Add prettier for formatting #79
Conversation
| @@ -38,7 +38,8 @@ function transform(source, transforms, options) { | |||
| }, options); | |||
| transforms = transforms || Object.keys(transformations).map(k => transformations[k]); | |||
| transforms.forEach(f => f(jscodeshift, ast)); | |||
| return ast.toSource(recastOptions); | |||
| const astStr = ast.toSource(recastOptions); | |||
There was a problem hiding this comment.
I wouldn't call it astStr since. Let's call it transformedSource?
There was a problem hiding this comment.
+1 Yes DAMP (Descriptive and Meaningful Phrases) ftw.
| @@ -38,7 +38,8 @@ function transform(source, transforms, options) { | |||
| }, options); | |||
| transforms = transforms || Object.keys(transformations).map(k => transformations[k]); | |||
| transforms.forEach(f => f(jscodeshift, ast)); | |||
| return ast.toSource(recastOptions); | |||
| const astStr = ast.toSource(recastOptions); | |||
| return prettier.format(astStr, {singleQuote: true, tabWidth: 2}); | |||
There was a problem hiding this comment.
Users should be able to pass prettier options. Also, some options like singleQuote and tabWidth might overlap recast options. We need to respect both or remove recast options completely.
There was a problem hiding this comment.
Lets drop recast options. Recast pretty print is not reliable. I'll add prettier options instead
| @@ -31,7 +31,8 @@ module.exports = { | |||
| `; | |||
|
|
|||
| describe('transform', () => { | |||
| it('should not transform if no transformations defined', () => { | |||
| // WIll not be equal unless input is also from prettier | |||
| xit('should not transform if no transformations defined', () => { | |||
There was a problem hiding this comment.
Right, that's pretty much an issue to me. We only should apply prettier if we have transformed something.
There was a problem hiding this comment.
Shall we applly prettier after showing diff to user? I'm not sure how to check if a transformation happened.. can AST's be compared?
There was a problem hiding this comment.
I think it can be done by checking the return value. Check out jscodeshift docs since I'm on mobile now
| @@ -166,9 +166,11 @@ if(argv.init) { | |||
| if (!filePaths.length) { | |||
| throw new Error('Please specify a path to your webpack config'); | |||
| } | |||
| const outputConfigName = filePaths[1] || `${filePaths[0]}v2.js`; | |||
There was a problem hiding this comment.
This will make git diff impossible and it will require an extra step from users to remove the old config and rename the new. IMO it defeats the purpose of using this tool. Why would someone like to keep the old config?
There was a problem hiding this comment.
Yes you are right. But can we keep this until testing is done?
There was a problem hiding this comment.
@okonet I could see a benefit of being able to have the old one left around especially while still a [WIP]. There were cases on initial trials of migrate where I had wished I specified outputConfigName beacuse I had lost the original, etc.
There was a problem hiding this comment.
At the least we should store it somewhere, but only as a temp trial for now, or if user specifies.
There was a problem hiding this comment.
Okay but in this case I'd suggest renaming the original config to something with webpack.config.orig.js in order to be able to run webpack without specifying the path to the new one in case of the default name. Either way, I guess for most people the next operation after transforming would be to try run the build and see if everything works. With a different name it's not possible.
There was a problem hiding this comment.
Looks like @okonet has all my thoughts covered in his suggested changes. This looks great overall.
| @@ -38,7 +38,8 @@ function transform(source, transforms, options) { | |||
| }, options); | |||
| transforms = transforms || Object.keys(transformations).map(k => transformations[k]); | |||
| transforms.forEach(f => f(jscodeshift, ast)); | |||
| return ast.toSource(recastOptions); | |||
| const astStr = ast.toSource(recastOptions); | |||
There was a problem hiding this comment.
+1 Yes DAMP (Descriptive and Meaningful Phrases) ftw.
| @@ -166,9 +166,11 @@ if(argv.init) { | |||
| if (!filePaths.length) { | |||
| throw new Error('Please specify a path to your webpack config'); | |||
| } | |||
| const outputConfigName = filePaths[1] || `${filePaths[0]}v2.js`; | |||
There was a problem hiding this comment.
At the least we should store it somewhere, but only as a temp trial for now, or if user specifies.
|
Better to do this after #92 |
What kind of change does this PR introduce?
Changing formatting to use prettier for #70
Did you add tests for your changes?
Existing tests are modified
If relevant, did you update the documentation?
Summary
By formatting with prettier, we can use recast as is without worrying about benjamn/recast#242.
Also due to this, we can add yarn support to our project!
Does this PR introduce a breaking change?
Yes, formatting changes in all snapshots.
Other information