From f6b5c94b5fc9ee959d9c02b05175a032260674e5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 21 Jul 2016 00:13:35 +0100 Subject: [PATCH 1/3] Make errors and warnings nicer --- config/eslint.js | 307 +++++++++++++--------------------- config/webpack.config.dev.js | 5 +- config/webpack.config.prod.js | 5 +- package.json | 4 +- scripts/start.js | 112 ++++++++++--- 5 files changed, 223 insertions(+), 210 deletions(-) diff --git a/config/eslint.js b/config/eslint.js index 941aba4ac24..2a140b9e18a 100644 --- a/config/eslint.js +++ b/config/eslint.js @@ -9,16 +9,22 @@ // Inspired by https://github.com/airbnb/javascript but less opinionated. -var OFF = 0; // rules that split the community (e.g. semicolons) -var WARNING = 1; // style rules accepted by the majority of popular styleguides -var ERROR = 2; // rules that prevent common mistakes +// We use eslint-loader so even warnings are very visibile. +// This is why we only use "WARNING" level for potential errors, +// and we don't use "ERROR" level at all. + +// In the future, we might create a separate list of rules for production. +// It would probably be more strict. + +var WARNING = 1; module.exports = { root: true, parser: 'babel-eslint', - plugins: ['react', 'import'], + // import plugin is termporarily disabled, scroll below to see why + plugins: ['react'/*, 'import'*/], env: { es6: true, @@ -51,78 +57,40 @@ module.exports = { rules: { // http://eslint.org/docs/rules/ - 'array-callback-return': ERROR, - 'arrow-spacing': [WARNING, { before: true, after: true }], - 'block-scoped-var': WARNING, - 'block-spacing': [WARNING, 'always'], - 'brace-style': [WARNING, '1tbs', { allowSingleLine: true }], - 'comma-spacing': [WARNING, { before: false, after: true }], - 'comma-style': [WARNING, 'last'], - 'computed-property-spacing': [WARNING, 'never'], - curly: [WARNING, 'multi-line'], - 'default-case': [ERROR, { commentPattern: '^no default$' }], - 'dot-notation': [WARNING, { allowKeywords: true }], - 'dot-location': [ERROR, 'property'], - 'eol-last': WARNING, - eqeqeq: [ERROR, 'allow-null'], - 'generator-star-spacing': [WARNING, { before: false, after: true }], - 'guard-for-in': ERROR, - 'key-spacing': [WARNING, { beforeColon: false, afterColon: true }], - 'keyword-spacing': [WARNING, { - before: true, - after: true, - overrides: { - return: { after: true }, - throw: { after: true }, - case: { after: true } - } - }], - 'linebreak-style': [WARNING, 'unix'], - 'new-cap': [ERROR, { newIsCap: true }], - 'new-parens': ERROR, - 'newline-per-chained-call': [WARNING, { ignoreChainWithDepth: 4 }], - 'no-array-constructor': ERROR, - 'no-caller': ERROR, - 'no-case-declarations': WARNING, - 'no-cond-assign': [ERROR, 'always'], - 'no-confusing-arrow': [WARNING, { - allowParens: true, - }], - 'no-console': OFF, // TODO: enable for production? - 'no-constant-condition': WARNING, - 'no-const-assign': ERROR, - 'no-control-regex': ERROR, - 'no-debugger': WARNING, // TODO: enable for production? - 'no-delete-var': ERROR, - 'no-dupe-args': ERROR, - 'no-dupe-class-members': ERROR, - 'no-dupe-keys': ERROR, - 'no-duplicate-case': ERROR, - 'no-duplicate-imports': WARNING, - 'no-empty': [WARNING, { - allowEmptyCatch: true - }], - 'no-empty-character-class': ERROR, - 'no-empty-pattern': ERROR, - 'no-eval': ERROR, - 'no-ex-assign': ERROR, - 'no-extend-native': ERROR, - 'no-extra-bind': ERROR, - 'no-extra-boolean-cast': WARNING, - 'no-extra-label': ERROR, - 'no-extra-semi': WARNING, - 'no-fallthrough': ERROR, - 'no-func-assign': ERROR, - 'no-implied-eval': ERROR, - 'no-invalid-this': WARNING, - 'no-invalid-regexp': ERROR, - 'no-irregular-whitespace': WARNING, - 'no-iterator': ERROR, - 'no-label-var': ERROR, - 'no-labels': [ERROR, { allowLoop: false, allowSwitch: false }], - 'no-lone-blocks': ERROR, - 'no-loop-func': ERROR, - 'no-mixed-operators': [ERROR, { + 'array-callback-return': WARNING, + 'default-case': [WARNING, { commentPattern: '^no default$' }], + 'dot-location': [WARNING, 'property'], + eqeqeq: [WARNING, 'allow-null'], + 'guard-for-in': WARNING, + 'new-cap': [WARNING, { newIsCap: true }], + 'new-parens': WARNING, + 'no-array-constructor': WARNING, + 'no-caller': WARNING, + 'no-cond-assign': [WARNING, 'always'], + 'no-const-assign': WARNING, + 'no-control-regex': WARNING, + 'no-delete-var': WARNING, + 'no-dupe-args': WARNING, + 'no-dupe-class-members': WARNING, + 'no-dupe-keys': WARNING, + 'no-duplicate-case': WARNING, + 'no-empty-character-class': WARNING, + 'no-empty-pattern': WARNING, + 'no-eval': WARNING, + 'no-ex-assign': WARNING, + 'no-extend-native': WARNING, + 'no-extra-bind': WARNING, + 'no-extra-label': WARNING, + 'no-fallthrough': WARNING, + 'no-func-assign': WARNING, + 'no-implied-eval': WARNING, + 'no-invalid-regexp': WARNING, + 'no-iterator': WARNING, + 'no-label-var': WARNING, + 'no-labels': [WARNING, { allowLoop: false, allowSwitch: false }], + 'no-lone-blocks': WARNING, + 'no-loop-func': WARNING, + 'no-mixed-operators': [WARNING, { groups: [ ['+', '-', '*', '/', '%', '**'], ['&', '|', '^', '~', '<<', '>>', '>>>'], @@ -132,136 +100,101 @@ module.exports = { ], allowSamePrecedence: false }], - 'no-multi-spaces': WARNING, - 'no-multi-str': ERROR, - 'no-multiple-empty-lines': [WARNING, { max: 2, maxEOF: 1 }], - 'no-native-reassign': ERROR, - 'no-negated-in-lhs': ERROR, - 'no-nested-ternary': WARNING, - 'no-new': WARNING, - 'no-new-func': ERROR, - 'no-new-object': ERROR, - 'no-new-symbol': ERROR, - 'no-new-wrappers': ERROR, - 'no-obj-calls': ERROR, - 'no-octal': ERROR, - 'no-octal-escape': ERROR, - 'no-prototype-builtins': WARNING, - 'no-redeclare': ERROR, - 'no-regex-spaces': ERROR, + 'no-multi-str': WARNING, + 'no-native-reassign': WARNING, + 'no-negated-in-lhs': WARNING, + 'no-new-func': WARNING, + 'no-new-object': WARNING, + 'no-new-symbol': WARNING, + 'no-new-wrappers': WARNING, + 'no-obj-calls': WARNING, + 'no-octal': WARNING, + 'no-octal-escape': WARNING, + 'no-redeclare': WARNING, + 'no-regex-spaces': WARNING, 'no-restricted-syntax': [ - ERROR, + WARNING, 'LabeledStatement', 'WithStatement', ], - 'no-return-assign': ERROR, - 'no-script-url': ERROR, - 'no-self-assign': ERROR, - 'no-self-compare': ERROR, - 'no-sequences': ERROR, - 'no-shadow': WARNING, - 'no-shadow-restricted-names': ERROR, - 'no-spaced-func': WARNING, - 'no-sparse-arrays': ERROR, - 'no-this-before-super': ERROR, - 'no-throw-literal': ERROR, - 'no-trailing-spaces': WARNING, - 'no-undef': ERROR, - 'no-unexpected-multiline': ERROR, - 'no-unneeded-ternary': [WARNING, { defaultAssignment: false }], - 'no-unreachable': ERROR, - 'no-unsafe-finally': WARNING, - 'no-unused-expressions': ERROR, - 'no-unused-labels': ERROR, - 'no-unused-vars': [ERROR, { vars: 'local', args: 'none' }], - 'no-use-before-define': [ERROR, 'nofunc'], - 'no-useless-computed-key': ERROR, - 'no-useless-concat': ERROR, - 'no-useless-constructor': ERROR, - 'no-useless-escape': ERROR, - 'no-useless-rename': [ERROR, { + 'no-return-assign': WARNING, + 'no-script-url': WARNING, + 'no-self-assign': WARNING, + 'no-self-compare': WARNING, + 'no-sequences': WARNING, + 'no-shadow-restricted-names': WARNING, + 'no-sparse-arrays': WARNING, + 'no-this-before-super': WARNING, + 'no-throw-literal': WARNING, + 'no-undef': WARNING, + 'no-unexpected-multiline': WARNING, + 'no-unreachable': WARNING, + 'no-unused-expressions': WARNING, + 'no-unused-labels': WARNING, + 'no-unused-vars': [WARNING, { vars: 'local', args: 'none' }], + 'no-use-before-define': [WARNING, 'nofunc'], + 'no-useless-computed-key': WARNING, + 'no-useless-concat': WARNING, + 'no-useless-constructor': WARNING, + 'no-useless-escape': WARNING, + 'no-useless-rename': [WARNING, { ignoreDestructuring: false, ignoreImport: false, ignoreExport: false, }], - 'no-var': WARNING, - 'no-with': ERROR, - 'no-whitespace-before-property': ERROR, - 'object-property-newline': [WARNING, { - allowMultiplePropertiesPerLine: true, - }], - 'operator-assignment': [ERROR, 'always'], - radix: ERROR, - 'require-yield': ERROR, - 'rest-spread-spacing': [ERROR, 'never'], - 'semi-spacing': [WARNING, { before: false, after: true }], - 'space-before-blocks': WARNING, - 'space-before-function-paren': [WARNING, { anonymous: 'always', named: 'never' }], - 'space-in-parens': [WARNING, 'never'], - 'space-infix-ops': WARNING, - 'space-unary-ops': [WARNING, { - words: true, - nonwords: false, - overrides: {}, - }], - 'spaced-comment': [WARNING, 'always', { - exceptions: ['-', '+'], - markers: ['=', '!'] - }], - strict: [ERROR, 'never'], - 'template-curly-spacing': WARNING, - 'unicode-bom': [ERROR, 'never'], - 'use-isnan': ERROR, - 'valid-typeof': ERROR, - 'yield-star-spacing': [WARNING, 'after'], - yoda: WARNING, + 'no-with': WARNING, + 'no-whitespace-before-property': WARNING, + 'operator-assignment': [WARNING, 'always'], + radix: WARNING, + 'require-yield': WARNING, + 'rest-spread-spacing': [WARNING, 'never'], + strict: [WARNING, 'never'], + 'unicode-bom': [WARNING, 'never'], + 'use-isnan': WARNING, + 'valid-typeof': WARNING, // https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/ - 'import/default': ERROR, - 'import/export': ERROR, - 'import/imports-first': WARNING, - 'import/named': ERROR, - 'import/namespace': ERROR, - 'import/no-amd': ERROR, - 'import/no-commonjs': WARNING, - 'import/no-duplicates': ERROR, - 'import/no-extraneous-dependencies': ERROR, - 'import/no-named-as-default': ERROR, - 'import/no-named-as-default-member': ERROR, - 'import/no-unresolved': [ERROR, { commonjs: true }], + // TODO: import rules are temporarily disabled because they don't play well + // with how eslint-loader only checks the file you change. So if module A + // imports module B, and B is missing a default export, the linter will + // record this as an issue in module A. Now if you fix module B, the linter + // will not be aware that it needs to re-lint A as well, so the error + // will stay until the next restart, which is really confusing. + + // This is probably fixable with a patch to eslint-loader. + // When file A is saved, we want to invalidate all files that import it + // *and* that currently have lint errors. This should fix the problem. + + // 'import/default': WARNING, + // 'import/export': WARNING, + // 'import/named': WARNING, + // 'import/namespace': WARNING, + // 'import/no-amd': WARNING, + // 'import/no-duplicates': WARNING, + // 'import/no-extraneous-dependencies': WARNING, + // 'import/no-named-as-default': WARNING, + // 'import/no-named-as-default-member': WARNING, + // 'import/no-unresolved': [WARNING, { commonjs: true }], // https://github.com/yannickcr/eslint-plugin-react/tree/master/docs/rules - 'react/jsx-equals-spacing': [ERROR, 'never'], + 'react/jsx-equals-spacing': [WARNING, 'never'], 'react/jsx-handler-names': [WARNING, { eventHandlerPrefix: 'handle', eventHandlerPropPrefix: 'on', }], - 'react/jsx-key': WARNING, - 'react/jsx-no-duplicate-props': [ERROR, { ignoreCase: true }], - 'react/jsx-no-undef': ERROR, - 'react/jsx-pascal-case': [ERROR, { + 'react/jsx-no-duplicate-props': [WARNING, { ignoreCase: true }], + 'react/jsx-no-undef': WARNING, + 'react/jsx-pascal-case': [WARNING, { allowAllCaps: true, ignore: [], }], - 'react/jsx-space-before-closing': WARNING, - 'react/jsx-uses-react': [ERROR, { pragma: 'React' }], - 'react/jsx-uses-vars': ERROR, - 'react/no-deprecated': ERROR, - 'react/no-did-mount-set-state': [WARNING, 'allow-in-func'], - 'react/no-did-update-set-state': [WARNING, 'allow-in-func'], - 'react/no-direct-mutation-state': ERROR, - 'react/no-is-mounted': ERROR, - 'react/no-multi-comp': [WARNING, { ignoreStateless: true }], - 'react/no-string-refs': WARNING, - 'react/prefer-es6-class': OFF, // TODO: revisit after updating docs - 'react/prefer-stateless-function': OFF, // TODO: revisit after updating docs - 'react/react-in-jsx-scope': ERROR, - 'react/require-render-return': ERROR, - 'react/wrap-multilines': [WARNING, { - declaration: true, - assignment: true, - return: true - }] + 'react/jsx-uses-react': WARNING, + 'react/jsx-uses-vars': WARNING, + 'react/no-deprecated': WARNING, + 'react/no-direct-mutation-state': WARNING, + 'react/no-is-mounted': WARNING, + 'react/react-in-jsx-scope': WARNING, + 'react/require-render-return': WARNING } }; diff --git a/config/webpack.config.dev.js b/config/webpack.config.dev.js index 44d5cec925b..e0e472c3b2b 100644 --- a/config/webpack.config.dev.js +++ b/config/webpack.config.dev.js @@ -21,7 +21,7 @@ module.exports = { entry: [ require.resolve('webpack-dev-server/client') + '?http://localhost:3000', require.resolve('webpack/hot/dev-server'), - './src/index.js' + './src/index' ], output: { // Next line is not used in dev but WebpackDevServer crashes without it: @@ -30,6 +30,9 @@ module.exports = { filename: 'bundle.js', publicPath: '/' }, + resolve: { + extensions: ['', '.js'], + }, resolveLoader: { root: path.join(__dirname, '..', 'node_modules'), moduleTemplates: ['*-loader'] diff --git a/config/webpack.config.prod.js b/config/webpack.config.prod.js index 74b06600831..35951cb4a2c 100644 --- a/config/webpack.config.prod.js +++ b/config/webpack.config.prod.js @@ -19,7 +19,7 @@ var relative = isInNodeModules ? '../../..' : '..'; module.exports = { bail: true, devtool: 'source-map', - entry: './src/index.js', + entry: './src/index', output: { path: path.resolve(__dirname, relative, 'build'), filename: '[name].[hash].js', @@ -27,6 +27,9 @@ module.exports = { // Good news: we can infer it from package.json :-) publicPath: '/' }, + resolve: { + extensions: ['', '.js'], + }, resolveLoader: { root: path.join(__dirname, '..', 'node_modules'), moduleTemplates: ['*-loader'] diff --git a/package.json b/package.json index 85f7f7f9afa..41437b1e51b 100644 --- a/package.json +++ b/package.json @@ -38,6 +38,7 @@ "babel-preset-es2015": "6.9.0", "babel-preset-es2016": "6.11.3", "babel-preset-react": "6.11.1", + "chalk": "^1.1.3", "cross-spawn": "4.0.0", "css-loader": "0.23.1", "eslint": "3.1.1", @@ -71,6 +72,7 @@ "babel-preset-es2015", "babel-preset-es2016", "babel-preset-react", + "chalk", "cross-spawn", "css-loader", "eslint", @@ -88,4 +90,4 @@ "webpack", "webpack-dev-server" ] -} +} \ No newline at end of file diff --git a/scripts/start.js b/scripts/start.js index 7623b9abf41..0efc9eb568b 100644 --- a/scripts/start.js +++ b/scripts/start.js @@ -9,6 +9,7 @@ process.env.NODE_ENV = 'development'; +var chalk = require('chalk'); var webpack = require('webpack'); var WebpackDevServer = require('webpack-dev-server'); var config = require('../config/webpack.config.dev'); @@ -18,7 +19,7 @@ var opn = require('opn'); var handleCompile; if (process.argv[2] === '--smoke-test') { handleCompile = function (err, stats) { - if (err || stats.toJson().errors.length || stats.toJson().warnings.length) { + if (err || stats.hasErrors() || stats.hasWarnings()) { process.exit(1); } else { process.exit(0); @@ -26,31 +27,102 @@ if (process.argv[2] === '--smoke-test') { }; } -new WebpackDevServer(webpack(config, handleCompile), { - publicPath: config.output.publicPath, +var friendlySyntaxErrorLabel = 'Syntax error:'; + +function isLikelyASyntaxError(message) { + return message.indexOf(friendlySyntaxErrorLabel) !== -1; +} + +// This is a little hacky. +// It would be easier if webpack provided a rich error object. + +function formatMessage(message) { + try { + return message + // Make some common errors shorter: + .replace( + // Babel syntax error + 'Module build failed: SyntaxError:', + friendlySyntaxErrorLabel + ) + .replace( + // Webpack file not found error + /Module not found: Error: Cannot resolve 'file' or 'directory'/, + 'Module not found:' + ) + // Internal stacks are generally useless so we strip them + .replace(/^\s*at\s.*\(.*:\d+:\d+\.*\).*\n/gm, '') // at ... (...:x:y) + // Webpack loader names obscure CSS filenames + .replace('./~/css-loader!./~/postcss-loader!', ''); + } catch (err) { + // Shouldn't happen but be extra careful. + return message; + } +} + +var compiler = webpack(config, handleCompile); +compiler.plugin('done', function (stats) { + // Clear the console and reset the cursor + process.stdout.write('\x1B[2J\x1B[0f'); + + var hasErrors = stats.hasErrors(); + var hasWarnings = stats.hasWarnings(); + + if (!hasErrors && !hasWarnings) { + console.log('Compiled successfully!'); + console.log('View the app at http://localhost:3000/'); + return; + } + + var json = stats.toJson(); + var formattedErrors = json.errors.map(message => + 'Error in ' + formatMessage(message) + ); + var formattedWarnings = json.warnings.map(message => + 'Warning in ' + formatMessage(message) + ); + + if (hasErrors) { + console.log('Compilation failed due to errors.'); + console.log(); + if (formattedErrors.some(isLikelyASyntaxError)) { + // If there are any syntax errors, show just them. + // This prevents a confusing ESLint parsing error + // preceding a much more useful Babel syntax error. + formattedErrors = formattedErrors.filter(isLikelyASyntaxError); + } + formattedErrors.forEach(message => { + console.log(message); + console.log(); + }); + // If errors exist, ignore warnings. + return; + } + + if (hasWarnings) { + console.log('Compiled with warnings.'); + console.log(); + formattedWarnings.forEach(message => { + console.log(message); + console.log(); + }); + + console.log('You may use special comments to disable some warnings.'); + console.log('Use ' + chalk.yellow('// eslint-disable-next-line') + ' to ignore the next line.'); + console.log('Use ' + chalk.yellow('/* eslint-disable */') + ' to ignore all warnings in a file.'); + } +}); + +new WebpackDevServer(compiler, { historyApiFallback: true, hot: true, // Note: only CSS is currently hot reloaded - stats: { - hash: false, - version: false, - timings: false, - assets: false, - chunks: false, - modules: false, - reasons: false, - children: false, - source: false, - publicPath: false, - colors: true, - errors: true, - errorDetails: true, - warnings: true, - } + publicPath: config.output.publicPath, + quiet: true }).listen(3000, 'localhost', function (err, result) { if (err) { return console.log(err); } - console.log('Running development server at http://localhost:3000/'); + console.log('Starting the development server...'); opn('http://localhost:3000/'); }); From e5da67a6382d0045cd982f9a946339f9e1a8fd5d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 21 Jul 2016 04:53:23 +0100 Subject: [PATCH 2/3] Fix license stripping to not be eager --- scripts/eject.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/eject.js b/scripts/eject.js index 7955f15cbcc..bf4c42ff05a 100644 --- a/scripts/eject.js +++ b/scripts/eject.js @@ -70,7 +70,7 @@ prompt('Are you sure you want to eject? This action is permanent. [y/N]', functi console.log('Copying ' + file + ' to ' + hostPath); var content = fs.readFileSync(path.join(selfPath, file), 'utf8'); // Remove license header - content = content.replace(/\/\*[\s\S]+\*\//, '').trim() + '\n'; + content = content.replace(/^\/\*\*(\*(?!\/)|[^*])*\*\//, '').trim() + '\n'; fs.writeFileSync(path.join(hostPath, file), content); }); From 81d28da3a0d294b9b8b858755e9602355db1165e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 21 Jul 2016 05:04:00 +0100 Subject: [PATCH 3/3] Minor tweaks --- scripts/start.js | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/scripts/start.js b/scripts/start.js index 0efc9eb568b..0edd941caa4 100644 --- a/scripts/start.js +++ b/scripts/start.js @@ -37,27 +37,22 @@ function isLikelyASyntaxError(message) { // It would be easier if webpack provided a rich error object. function formatMessage(message) { - try { - return message - // Make some common errors shorter: - .replace( - // Babel syntax error - 'Module build failed: SyntaxError:', - friendlySyntaxErrorLabel - ) - .replace( - // Webpack file not found error - /Module not found: Error: Cannot resolve 'file' or 'directory'/, - 'Module not found:' - ) - // Internal stacks are generally useless so we strip them - .replace(/^\s*at\s.*\(.*:\d+:\d+\.*\).*\n/gm, '') // at ... (...:x:y) - // Webpack loader names obscure CSS filenames - .replace('./~/css-loader!./~/postcss-loader!', ''); - } catch (err) { - // Shouldn't happen but be extra careful. - return message; - } + return message + // Make some common errors shorter: + .replace( + // Babel syntax error + 'Module build failed: SyntaxError:', + friendlySyntaxErrorLabel + ) + .replace( + // Webpack file not found error + /Module not found: Error: Cannot resolve 'file' or 'directory'/, + 'Module not found:' + ) + // Internal stacks are generally useless so we strip them + .replace(/^\s*at\s.*\(.*:\d+:\d+\.*\).*\n/gm, '') // at ... (...:x:y) + // Webpack loader names obscure CSS filenames + .replace('./~/css-loader!./~/postcss-loader!', ''); } var compiler = webpack(config, handleCompile); @@ -69,8 +64,10 @@ compiler.plugin('done', function (stats) { var hasWarnings = stats.hasWarnings(); if (!hasErrors && !hasWarnings) { - console.log('Compiled successfully!'); - console.log('View the app at http://localhost:3000/'); + console.log(chalk.green('Compiled successfully!')); + console.log(); + console.log('The app is running at http://localhost:3000/'); + console.log(); return; } @@ -83,7 +80,7 @@ compiler.plugin('done', function (stats) { ); if (hasErrors) { - console.log('Compilation failed due to errors.'); + console.log(chalk.red('There were errors compiling.')); console.log(); if (formattedErrors.some(isLikelyASyntaxError)) { // If there are any syntax errors, show just them. @@ -100,7 +97,7 @@ compiler.plugin('done', function (stats) { } if (hasWarnings) { - console.log('Compiled with warnings.'); + console.log(chalk.yellow('Compiled with warnings.')); console.log(); formattedWarnings.forEach(message => { console.log(message);