Merge pull request #21 from Automattic/fix/19-render-child

jsx-classname-namespace: Mark element in render as root only if returned
Automattic · Dec 5, 2018 · 3b2284e · 3b2284e
1 parent 2e76cd0
commit 3b2284e
Show file tree

Hide file tree

Showing 3 changed files with 140 additions and 1 deletion.
diff --git a/packages/eslint-plugin-wpcalypso/CHANGELOG.md b/packages/eslint-plugin-wpcalypso/CHANGELOG.md
@@ -1,6 +1,7 @@
 #### v2.0.0 (---)
 
 - Breaking: Required Node version increased to >=0.12.0
+- Fix: `jsx-classname-namespace` can accurately validate elements assigned to variables within render ([#21](https://github.com/Automattic/eslint-plugin-wpcalypso/pull/21))
 - Fix: `npm test` is now run synchronously so it exits with a non-zero code on failure
 - Fix: Replace ES2015 variable (`let`) declarations to accommodate older Node versions
 - Fix: Remove unintended debugging statement from i18n-no-newlines rule

diff --git a/packages/eslint-plugin-wpcalypso/lib/rules/jsx-classname-namespace.js b/packages/eslint-plugin-wpcalypso/lib/rules/jsx-classname-namespace.js
@@ -86,14 +86,63 @@ var rule = module.exports = function( context ) {
 		].indexOf( node.type );
 	}
 
+	function getFunctionReturnValue( node ) {
+		var i, bodyNode;
+
+		// An arrow function expression is one whose return statement is
+		// implicit. It does not have a body block.
+		//
+		// See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions
+		if ( 'ArrowFunctionExpression' === node.type && node.expression ) {
+			return node.body;
+		}
+
+		for ( i = 0; i < node.body.body.length; i++ ) {
+			bodyNode = node.body.body[ i ];
+			if ( 'ReturnStatement' === bodyNode.type ) {
+				return bodyNode.argument;
+			}
+		}
+	}
+
+	function isSameIdentifier( nodeA, nodeB ) {
+		if ( ! nodeA || ! nodeB ) {
+			return false;
+		}
+
+		if ( 'Identifier' !== nodeA.type || 'Identifier' !== nodeB.type ) {
+			return false;
+		}
+
+		return nodeA.name === nodeB.name;
+	}
+
 	function isRootRenderedElement( node, filename ) {
-		var parent = node.parent.parent,
+		var element, isElementReturnArg, elementAssignedIdentifier, parent,
 			functionExpression, functionName, isRoot;
 
 		if ( ! REGEXP_INDEX_PATH.test( filename ) ) {
 			return false;
 		}
 
+		element = node.parent.parent;
+
+		switch ( element.parent.type ) {
+			case 'ArrowFunctionExpression':
+				isElementReturnArg = element.parent.expression;
+				break;
+
+			case 'ReturnStatement':
+				isElementReturnArg = true;
+				break;
+
+			case 'VariableDeclarator':
+				elementAssignedIdentifier = element.parent.id;
+				break;
+		}
+
+		parent = element;
+
 		do {
 			parent = parent.parent;
 
@@ -139,6 +188,13 @@ var rule = module.exports = function( context ) {
 							isRoot = true;
 						}
 				}
+
+				// If we suspect the element is the root, confirm that it's the
+				// return value of the function
+				if ( isRoot && ! isElementReturnArg &&
+						! isSameIdentifier( elementAssignedIdentifier, getFunctionReturnValue( parent ) ) ) {
+					isRoot = false;
+				}
 			}
 
 			// If we've exhausted parent options, check to see whether exports

diff --git a/packages/eslint-plugin-wpcalypso/tests/lib/rules/jsx-classname-namespace.js b/packages/eslint-plugin-wpcalypso/tests/lib/rules/jsx-classname-namespace.js
@@ -57,17 +57,33 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
 			filename: '/tmp/foo/index.js'
 		},
+		{
+			code: 'export default function() { const foo = <Foo className="foo" />; return foo; }',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
 		{
 			code: 'export default function() { return <Foo className="quux foo" />; }',
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
 			filename: '/tmp/foo/index.js'
 		},
+		{
+			code: 'export default function() { const child = <div className="foo__child" />; return <Foo className="foo">{ child }</Foo>; }',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
 		{
 			code: 'export default () => <Foo className="foo" />;',
 			env: { es6: true },
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
 			filename: '/tmp/foo/index.js'
 		},
+		{
+			code: 'export default () => { return <Foo className="foo" />; }',
+			env: { es6: true },
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
 		{
 			code: 'const Foo = () => <Foo className="foo" />; export default Foo;',
 			env: { es6: true },
@@ -119,12 +135,28 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
 			filename: '/tmp/foo/index.js'
 		},
+		{
+			code: 'export default React.createClass( { render: function() { const foo = <Foo className="foo" />; return foo; } } );',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
+		{
+			code: 'export default React.createClass( { render: function() { return ( <Foo className="foo" /> ); } } );',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
 		{
 			code: 'export default React.createClass( { render() { return <Foo className="foo"><div className="foo__child" /></Foo>; } } );',
 			env: { es6: true },
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
 			filename: '/tmp/foo/index.js'
 		},
+		{
+			code: 'export default React.createClass( { render() { const child = <div className="foo__child" />; return <Foo className="foo">{ child }</Foo>; } } );',
+			env: { es6: true },
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js'
+		},
 		{
 			code: 'const isOk = true; export default React.createClass( { render() { return <Foo className="foo">{ isOk && <div className="foo__child" /> }</Foo>; } } );',
 			env: { es6: true },
@@ -201,6 +233,22 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 				message: EXPECTED_FOO_ERROR
 			} ]
 		},
+		{
+			code: 'export default function() { const foo = <Foo className="foobar" />; return foo; }',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_ERROR
+			} ]
+		},
+		{
+			code: 'export default function() { const child = <div className="foo" />; return <Foo className="foo">{ child }</Foo>; }',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_PREFIX_ERROR
+			} ]
+		},
 		{
 			code: 'export default function() { return <Foo className="quux foobar" />; }',
 			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
@@ -218,6 +266,15 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 				message: EXPECTED_FOO_ERROR
 			} ]
 		},
+		{
+			code: 'export default () => { return <Foo className="foobar" />; }',
+			env: { es6: true },
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_ERROR
+			} ]
+		},
 		{
 			code: 'const Foo = () => <Foo className="foobar" />; export default Foo;',
 			env: { es6: true },
@@ -297,6 +354,22 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 				message: EXPECTED_FOO_ERROR
 			} ]
 		},
+		{
+			code: 'export default React.createClass( { render: function() { const foo = <Foo className="foobar" />; return foo; } } );',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_ERROR
+			} ]
+		},
+		{
+			code: 'export default React.createClass( { render: function() { return ( <Foo className="foobar" /> ); } } );',
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_ERROR
+			} ]
+		},
 		{
 			code: 'export default React.createClass( { render() { return <Foo className="foo"><div className="foobar__child" /></Foo>; } } );',
 			env: { es6: true },
@@ -306,6 +379,15 @@ EXPECTED_FOO_PREFIX_ERROR = formatMessage( rule.ERROR_MESSAGE, { expected: 'foo_
 				message: EXPECTED_FOO_PREFIX_ERROR
 			} ]
 		},
+		{
+			code: 'export default React.createClass( { render() { const child = <div className="foo" />; return <Foo className="foo">{ child }</Foo>; } } );',
+			env: { es6: true },
+			parserOptions: { ecmaFeatures: { jsx: true }, sourceType: 'module' },
+			filename: '/tmp/foo/index.js',
+			errors: [ {
+				message: EXPECTED_FOO_PREFIX_ERROR
+			} ]
+		},
 		{
 			code: 'const isOk = true; export default React.createClass( { render() { return <Foo className="foo">{ isOk && <div className="foobar__child" /> }</Foo>; } } );',
 			env: { es6: true },