Skip to content
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.

Sign up for GitHub

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

feat: Add prettier for formatting #79

Closed
wants to merge 2 commits into from
Closed

feat: Add prettier for formatting #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9828582
feat: Add prettier for formatting
Mar 2, 2017
3ff5edd
fix
Mar 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion bin/webpack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right. But can we keep this until testing is done?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the least we should store it somewhere, but only as a temp trial for now, or if user specifies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const inputConfigPath = path.resolve(process.cwd(), filePaths[0]);
const outputConfigPath = path.resolve(process.cwd(), outputConfigName);

require('../lib/migrate.js')(inputConfigPath, inputConfigPath);
require('../lib/migrate.js')(inputConfigPath, outputConfigPath);
} else {
processOptions(yargs,argv);
}
169 changes: 83 additions & 86 deletions lib/transformations/__snapshots__/index.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,102 +1,99 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`transform should respect recast options 1`] = `
"
module.exports = {
devtool: 'eval',
entry: [
'./src/index'
],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [{
test: /.js$/,
use: \\"babel\\",
include: path.join(__dirname, 'src')
}]
},
resolve: {
modules: ['node_modules', path.resolve('/src')],
},
plugins: [
new webpack.optimize.UglifyJsPlugin({
sourceMap: true,
}),
new webpack.optimize.LoaderOptionsPlugin({
\\"debug\\": true,
\\"minimize\\": true,
})
],
debug: true
"module.exports = {
devtool: 'eval',
entry: ['./src/index'],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [
{
test: /.js$/,
use: 'babel',
include: path.join(__dirname, 'src')
}
]
},
resolve: {
modules: ['node_modules', path.resolve('/src')]
},
plugins: [
new webpack.optimize.UglifyJsPlugin({
sourceMap: true
}),
new webpack.optimize.LoaderOptionsPlugin({
debug: true,
minimize: true
})
],
debug: true
};
"
`;

exports[`transform should transform only using specified transformations 1`] = `
"
module.exports = {
devtool: 'eval',
entry: [
'./src/index'
],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [{
test: /.js$/,
use: ['babel'],
include: path.join(__dirname, 'src')
}]
},
resolve: {
root: path.resolve('/src'),
modules: ['node_modules']
},
plugins: [
new webpack.optimize.UglifyJsPlugin(),
new webpack.optimize.OccurrenceOrderPlugin()
],
debug: true
"module.exports = {
devtool: 'eval',
entry: ['./src/index'],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [
{
test: /.js$/,
use: ['babel'],
include: path.join(__dirname, 'src')
}
]
},
resolve: {
root: path.resolve('/src'),
modules: ['node_modules']
},
plugins: [
new webpack.optimize.UglifyJsPlugin(),
new webpack.optimize.OccurrenceOrderPlugin()
],
debug: true
};
"
`;

exports[`transform should transform using all transformations 1`] = `
"
module.exports = {
devtool: 'eval',
entry: [
'./src/index'
],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [{
test: /.js$/,
use: 'babel',
include: path.join(__dirname, 'src')
}]
},
resolve: {
modules: ['node_modules', path.resolve('/src')]
},
plugins: [
new webpack.optimize.UglifyJsPlugin({
sourceMap: true
}),
new webpack.optimize.LoaderOptionsPlugin({
'debug': true,
'minimize': true
})
],
debug: true
"module.exports = {
devtool: 'eval',
entry: ['./src/index'],
output: {
path: path.join(__dirname, 'dist'),
filename: 'index.js'
},
module: {
rules: [
{
test: /.js$/,
use: 'babel',
include: path.join(__dirname, 'src')
}
]
},
resolve: {
modules: ['node_modules', path.resolve('/src')]
},
plugins: [
new webpack.optimize.UglifyJsPlugin({
sourceMap: true
}),
new webpack.optimize.LoaderOptionsPlugin({
debug: true,
minimize: true
})
],
debug: true
};
"
`;
5 changes: 3 additions & 2 deletions lib/transformations/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const jscodeshift = require('jscodeshift');

const prettier = require('prettier');
const loadersTransform = require('./loaders/loaders');
const resolveTransform = require('./resolve/resolve');
const removeJsonLoaderTransform = require('./removeJsonLoader/removeJsonLoader');
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't call it astStr since. Let's call it transformedSource?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Yes DAMP (Descriptive and Meaningful Phrases) ftw.

return prettier.format(astStr, {singleQuote: true, tabWidth: 2});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets drop recast options. Recast pretty print is not reliable. I'll add prettier options instead

}

module.exports = {
Expand Down
4 changes: 3 additions & 1 deletion lib/transformations/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's pretty much an issue to me. We only should apply prettier if we have transformed something.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be done by checking the return value. Check out jscodeshift docs since I'm on mobile now

const output = transform(input, []);
expect(output).toEqual(input);
});
Expand All @@ -46,6 +47,7 @@ describe('transform', () => {
expect(output).toMatchSnapshot();
});

//TODO - What will we depend on? Recast or prettier options?
it('should respect recast options', () => {
const output = transform(input, undefined, {
quote: 'double',
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@
"jscodeshift": "^0.3.30",
"loader-utils": "^0.2.16",
"lodash": "^4.17.4",
"recast": "git://github.com/kalcifer/recast.git#bug/allowbreak",
"prettier": "^0.20.0",
"recast": "^0.11.22",
"rx": "^4.1.0",
"supports-color": "^3.1.2",
"webpack": "^2.2.0-rc.0",
Expand Down
Loading