From 47e1e25d8f3fd06b46514caca9b1233014b962b1 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 27 May 2015 14:45:58 -0700 Subject: [PATCH 1/5] Use babel-eslint and update eslint - Removes esprima-fb dependency - Tightens up eslintrc with some rules we were pretty-much following anyways. - Adds pretty colors to the `grunt lint` output - Breaks block-scoped-var :( --- .eslintrc | 18 ++++++++++++------ grunt/tasks/eslint.js | 3 ++- grunt/tasks/release.js | 2 +- npm-react-codemod/transforms/class.js | 6 +++--- npm-react-codemod/transforms/findDOMNode.js | 2 +- .../transforms/pure-render-mixin.js | 2 +- .../transforms/utils/ReactUtils.js | 4 ++-- package.json | 4 ++-- src/addons/ReactFragment.js | 13 +++++++++---- src/isomorphic/classic/element/ReactElement.js | 1 + .../dom/client/ReactBrowserEventEmitter.js | 4 ++-- .../client/eventPlugins/ChangeEventPlugin.js | 2 +- src/shared/utils/Transaction.js | 15 +++++++++------ test/lib/postDataToURL.browser.js | 4 ++-- 14 files changed, 48 insertions(+), 32 deletions(-) diff --git a/.eslintrc b/.eslintrc index 347734de88a..7bca012c896 100644 --- a/.eslintrc +++ b/.eslintrc @@ -1,5 +1,5 @@ --- -parser: esprima-fb +parser: babel-eslint env: browser: true @@ -17,16 +17,18 @@ rules: strict: 2 # We actually have a transform to support this and we fix this for bundled # releases but not for the npm package, so enforce it strictly - no-comma-dangle: 2 + comma-dangle: [2, never] # Make this a warning for now. We do this in a few places so we might need to # disable no-unused-expressions: 2 - block-scoped-var: 2 eol-last: 2 dot-notation: 2 + dot-location: [2, property] consistent-return: 2 no-unused-vars: [2, args: none] - quotes: [2, 'single'] + quotes: [2, single] + no-shadow: 2 + no-multi-spaces: 2 # WARNINGS # This is the only one that's hard to track since we don't lint just changes. @@ -47,5 +49,9 @@ rules: # We do this in a few places to align values key-spacing: 0 - # DISABLED. These currently cause errors when running. - no-multi-spaces: 0 + # BROKEN. We'd like to turn these back on. + block-scoped-var: 0 # causes a ton of noise, eslint is too picky? + # on-empty is broken in babel-eslint: + # https://github.com/babel/babel-eslint/issues/62 + # commented blocks shouldn't be marked as empty blocks. + no-empty: 0 diff --git a/grunt/tasks/eslint.js b/grunt/tasks/eslint.js index b2b56903943..861962a9011 100644 --- a/grunt/tasks/eslint.js +++ b/grunt/tasks/eslint.js @@ -6,7 +6,8 @@ module.exports = function() { var done = this.async(); grunt.util.spawn({ cmd: 'node_modules/.bin/eslint', - args: ['.'] + args: ['.'], + opts: {stdio: 'inherit'} // allows colors to passthrough }, function(err, result, code) { if (err) { grunt.log.error('Lint failed'); diff --git a/grunt/tasks/release.js b/grunt/tasks/release.js index dde3f809364..97619cfc95f 100644 --- a/grunt/tasks/release.js +++ b/grunt/tasks/release.js @@ -15,7 +15,7 @@ var EXAMPLES_PATH = 'examples/'; var EXAMPLES_GLOB = [EXAMPLES_PATH + '**/*.*']; var STARTER_PATH = 'starter/'; -var STARTER_GLOB = [STARTER_PATH + '/**/*.*']; +var STARTER_GLOB = [STARTER_PATH + '/**/*.*']; var STARTER_BUILD_PATH = 'build/starter/'; diff --git a/npm-react-codemod/transforms/class.js b/npm-react-codemod/transforms/class.js index 4acc162781b..1a0d291c4c9 100644 --- a/npm-react-codemod/transforms/class.js +++ b/npm-react-codemod/transforms/class.js @@ -8,7 +8,7 @@ * */ -/*eslint-disable no-comma-dangle*/ +/*eslint-disable comma-dangle*/ 'use strict'; @@ -157,11 +157,11 @@ function updateReactCreateClassToES6(file, api, options) { ) .filter(isFunctionExpression); - const findAutobindNamesFor = (root, fnNames, literalOrIdentifier) => { + const findAutobindNamesFor = (subtree, fnNames, literalOrIdentifier) => { const node = literalOrIdentifier; const autobindNames = {}; - j(root) + j(subtree) .find(j.MemberExpression, { object: node.name ? { type: node.type, diff --git a/npm-react-codemod/transforms/findDOMNode.js b/npm-react-codemod/transforms/findDOMNode.js index 5d0cacc60db..c59334fba0d 100644 --- a/npm-react-codemod/transforms/findDOMNode.js +++ b/npm-react-codemod/transforms/findDOMNode.js @@ -8,7 +8,7 @@ * */ -/*eslint-disable no-comma-dangle*/ +/*eslint-disable comma-dangle*/ 'use strict'; diff --git a/npm-react-codemod/transforms/pure-render-mixin.js b/npm-react-codemod/transforms/pure-render-mixin.js index 5a6f6d4a951..fc30ad4ebd3 100644 --- a/npm-react-codemod/transforms/pure-render-mixin.js +++ b/npm-react-codemod/transforms/pure-render-mixin.js @@ -8,7 +8,7 @@ * */ -/*eslint-disable no-comma-dangle*/ +/*eslint-disable comma-dangle*/ 'use strict'; diff --git a/npm-react-codemod/transforms/utils/ReactUtils.js b/npm-react-codemod/transforms/utils/ReactUtils.js index 351d334bdf2..64dbedc06ac 100644 --- a/npm-react-codemod/transforms/utils/ReactUtils.js +++ b/npm-react-codemod/transforms/utils/ReactUtils.js @@ -8,7 +8,7 @@ * */ - /*eslint-disable no-comma-dangle*/ +/*eslint-disable comma-dangle*/ 'use strict'; @@ -47,7 +47,7 @@ module.exports = function(j) { const findReactCreateClass = path => path .findVariableDeclarators() - .filter(path => findReactCreateClassCallExpression(path).size() > 0); + .filter(decl => findReactCreateClassCallExpression(decl).size() > 0); const findReactCreateClassModuleExports = path => path diff --git a/package.json b/package.json index 0749dcdf0e1..22eb65e0377 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ }, "devDependencies": { "babel": "^5.3.3", + "babel-eslint": "^3.1.9", "benchmark": "~1.0.0", "browserify": "^9.0.3", "bundle-collapser": "^1.1.1", @@ -38,8 +39,7 @@ "derequire": "^2.0.0", "envify": "^3.0.0", "es5-shim": "^4.0.0", - "eslint": "^0.14.1", - "esprima-fb": "^15001.1.0-dev-harmony-fb", + "eslint": "^0.21.2", "grunt": "~0.4.2", "grunt-cli": "^0.1.13", "grunt-compare-size": "~0.4.0", diff --git a/src/addons/ReactFragment.js b/src/addons/ReactFragment.js index 19cb40b4bda..40e3dc1789e 100644 --- a/src/addons/ReactFragment.js +++ b/src/addons/ReactFragment.js @@ -23,10 +23,13 @@ var warning = require('warning'); * create a keyed fragment. The resulting data structure is opaque, for now. */ +var fragmentKey; +var didWarnKey; +var canWarnForReactFragment; + if (__DEV__) { - var fragmentKey = '_reactFragment'; - var didWarnKey = '_reactDidWarn'; - var canWarnForReactFragment = false; + fragmentKey = '_reactFragment'; + didWarnKey = '_reactDidWarn'; try { // Feature test. Don't even try to issue this warning if we can't use @@ -49,7 +52,9 @@ if (__DEV__) { ); canWarnForReactFragment = true; - } catch (x) { } + } catch (x) { + canWarnForReactFragment = false; + } var proxyPropertyAccessWithWarning = function(obj, key) { Object.defineProperty(obj, key, { diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 4760c6311d3..afaede1808e 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -117,6 +117,7 @@ var ReactElement = function(type, key, ref, owner, context, props) { writable: true }); } catch (x) { + // IE will fail on defineProperty } this._store.validated = false; diff --git a/src/renderers/dom/client/ReactBrowserEventEmitter.js b/src/renderers/dom/client/ReactBrowserEventEmitter.js index d7f5da9616c..2805910b801 100644 --- a/src/renderers/dom/client/ReactBrowserEventEmitter.js +++ b/src/renderers/dom/client/ReactBrowserEventEmitter.js @@ -212,8 +212,8 @@ var ReactBrowserEventEmitter = assign({}, ReactEventEmitterMixin, { listenTo: function(registrationName, contentDocumentHandle) { var mountAt = contentDocumentHandle; var isListening = getListeningForDocument(mountAt); - var dependencies = EventPluginRegistry. - registrationNameDependencies[registrationName]; + var dependencies = EventPluginRegistry + .registrationNameDependencies[registrationName]; var topLevelTypes = EventConstants.topLevelTypes; for (var i = 0; i < dependencies.length; i++) { diff --git a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js index df83f540e6b..52b1d00d739 100644 --- a/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/ChangeEventPlugin.js @@ -150,7 +150,7 @@ if (ExecutionEnvironment.canUseDOM) { * (For old IE.) Replacement getter/setter for the `value` property that gets * set on the active element. */ -var newValueProp = { +var newValueProp = { get: function() { return activeElementValueProp.get.call(this); }, diff --git a/src/shared/utils/Transaction.js b/src/shared/utils/Transaction.js index a487e8ae1de..a771e495365 100644 --- a/src/shared/utils/Transaction.js +++ b/src/shared/utils/Transaction.js @@ -141,6 +141,7 @@ var Mixin = { try { this.closeAll(0); } catch (err) { + // Ignore additional errors, only throw the original } } else { // Since `method` didn't throw, we don't want to silence the exception @@ -169,12 +170,13 @@ var Mixin = { null; } finally { if (this.wrapperInitData[i] === Transaction.OBSERVED_ERROR) { - // The initializer for wrapper i threw an error; initialize the - // remaining wrappers but silence any exceptions from them to ensure - // that the first error is the one to bubble up. + // The initializer for wrapper i threw an error; try { + // initialize the remaining wrappers, this.initializeAll(i + 1); } catch (err) { + // but silence any exceptions from them to ensure that the first + // error is the one to bubble up. } } } @@ -209,12 +211,13 @@ var Mixin = { errorThrown = false; } finally { if (errorThrown) { - // The closer for wrapper i threw an error; close the remaining - // wrappers but silence any exceptions from them to ensure that the - // first error is the one to bubble up. + // The closer for wrapper i threw an error; try { + // close the remaining wrappers, this.closeAll(i + 1); } catch (e) { + // but silence any exceptions from them to ensure that the first + // error is the one to bubble up. } } } diff --git a/test/lib/postDataToURL.browser.js b/test/lib/postDataToURL.browser.js index 5822ce16bba..ade2614e173 100644 --- a/test/lib/postDataToURL.browser.js +++ b/test/lib/postDataToURL.browser.js @@ -1,4 +1,4 @@ -/*eslint-disable brace-style*/ +/*eslint-disable brace-style, no-empty */ function createXMLHttpRequest() { try { return new XMLHttpRequest(); @@ -13,7 +13,7 @@ function createXMLHttpRequest() { } catch (e) {} } -/*eslint-enable brace-style*/ +/*eslint-enable brace-style, no-empty */ function getURLSync(url) { var request = createXMLHttpRequest(); From 7066f36d8681e107c1b4487ca5abf4fbc56c4a1a Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 27 May 2015 15:34:40 -0700 Subject: [PATCH 2/5] Revert addition of no-empty rule This also reverts the changes that were made to support this, excluding the changes in ReactFragment, as IMHO, it's more readable moving `canWarnForReactFragment = false` into the catch block. --- .eslintrc | 6 ++---- src/isomorphic/classic/element/ReactElement.js | 1 - src/shared/utils/Transaction.js | 15 ++++++--------- test/lib/postDataToURL.browser.js | 4 ++-- 4 files changed, 10 insertions(+), 16 deletions(-) diff --git a/.eslintrc b/.eslintrc index 7bca012c896..e09d4cb6b03 100644 --- a/.eslintrc +++ b/.eslintrc @@ -48,10 +48,8 @@ rules: no-use-before-define: 0 # We do this in a few places to align values key-spacing: 0 + # https://github.com/facebook/react/pull/3968 + no-empty: 0 # BROKEN. We'd like to turn these back on. block-scoped-var: 0 # causes a ton of noise, eslint is too picky? - # on-empty is broken in babel-eslint: - # https://github.com/babel/babel-eslint/issues/62 - # commented blocks shouldn't be marked as empty blocks. - no-empty: 0 diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index afaede1808e..4760c6311d3 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -117,7 +117,6 @@ var ReactElement = function(type, key, ref, owner, context, props) { writable: true }); } catch (x) { - // IE will fail on defineProperty } this._store.validated = false; diff --git a/src/shared/utils/Transaction.js b/src/shared/utils/Transaction.js index a771e495365..a487e8ae1de 100644 --- a/src/shared/utils/Transaction.js +++ b/src/shared/utils/Transaction.js @@ -141,7 +141,6 @@ var Mixin = { try { this.closeAll(0); } catch (err) { - // Ignore additional errors, only throw the original } } else { // Since `method` didn't throw, we don't want to silence the exception @@ -170,13 +169,12 @@ var Mixin = { null; } finally { if (this.wrapperInitData[i] === Transaction.OBSERVED_ERROR) { - // The initializer for wrapper i threw an error; + // The initializer for wrapper i threw an error; initialize the + // remaining wrappers but silence any exceptions from them to ensure + // that the first error is the one to bubble up. try { - // initialize the remaining wrappers, this.initializeAll(i + 1); } catch (err) { - // but silence any exceptions from them to ensure that the first - // error is the one to bubble up. } } } @@ -211,13 +209,12 @@ var Mixin = { errorThrown = false; } finally { if (errorThrown) { - // The closer for wrapper i threw an error; + // The closer for wrapper i threw an error; close the remaining + // wrappers but silence any exceptions from them to ensure that the + // first error is the one to bubble up. try { - // close the remaining wrappers, this.closeAll(i + 1); } catch (e) { - // but silence any exceptions from them to ensure that the first - // error is the one to bubble up. } } } diff --git a/test/lib/postDataToURL.browser.js b/test/lib/postDataToURL.browser.js index ade2614e173..5822ce16bba 100644 --- a/test/lib/postDataToURL.browser.js +++ b/test/lib/postDataToURL.browser.js @@ -1,4 +1,4 @@ -/*eslint-disable brace-style, no-empty */ +/*eslint-disable brace-style*/ function createXMLHttpRequest() { try { return new XMLHttpRequest(); @@ -13,7 +13,7 @@ function createXMLHttpRequest() { } catch (e) {} } -/*eslint-enable brace-style, no-empty */ +/*eslint-enable brace-style*/ function getURLSync(url) { var request = createXMLHttpRequest(); From ce22e5bcb1ebbfcd36019fb8d4aa23a82b0fec19 Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 27 May 2015 16:04:29 -0700 Subject: [PATCH 3/5] Comment-above instead of same line in eslintrc (sorry, habits!) --- .eslintrc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.eslintrc b/.eslintrc index e09d4cb6b03..7a0f9959503 100644 --- a/.eslintrc +++ b/.eslintrc @@ -52,4 +52,5 @@ rules: no-empty: 0 # BROKEN. We'd like to turn these back on. - block-scoped-var: 0 # causes a ton of noise, eslint is too picky? + # causes a ton of noise, eslint is too picky? + block-scoped-var: 0 From ed3b3b7934911937706bfc018f96d5fd60787c9e Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 27 May 2015 16:15:56 -0700 Subject: [PATCH 4/5] Change ReactBrowserEventEmitter line-split style --- src/renderers/dom/client/ReactBrowserEventEmitter.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderers/dom/client/ReactBrowserEventEmitter.js b/src/renderers/dom/client/ReactBrowserEventEmitter.js index 2805910b801..6f7951116e1 100644 --- a/src/renderers/dom/client/ReactBrowserEventEmitter.js +++ b/src/renderers/dom/client/ReactBrowserEventEmitter.js @@ -212,8 +212,8 @@ var ReactBrowserEventEmitter = assign({}, ReactEventEmitterMixin, { listenTo: function(registrationName, contentDocumentHandle) { var mountAt = contentDocumentHandle; var isListening = getListeningForDocument(mountAt); - var dependencies = EventPluginRegistry - .registrationNameDependencies[registrationName]; + var dependencies = + EventPluginRegistry.registrationNameDependencies[registrationName]; var topLevelTypes = EventConstants.topLevelTypes; for (var i = 0; i < dependencies.length; i++) { From a519ee118bdfc6c7b9229ba884e675c36ae97d0a Mon Sep 17 00:00:00 2001 From: Benjamin Woodruff Date: Wed, 27 May 2015 18:08:40 -0700 Subject: [PATCH 5/5] Replace PR link with description in eslintrc > Nit: can you actually mention why it's disabled and not just link to a > PR which has a lot of unrelated discussion. -- @zpao --- .eslintrc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.eslintrc b/.eslintrc index 7a0f9959503..d73fda086fc 100644 --- a/.eslintrc +++ b/.eslintrc @@ -48,7 +48,7 @@ rules: no-use-before-define: 0 # We do this in a few places to align values key-spacing: 0 - # https://github.com/facebook/react/pull/3968 + # It's nice to be able to leave catch blocks empty no-empty: 0 # BROKEN. We'd like to turn these back on.