From ac98c0f86aa4af5793e7006422edb697827b72a6 Mon Sep 17 00:00:00 2001 From: Dylan Oshima Date: Fri, 21 Aug 2020 23:09:11 -0700 Subject: [PATCH] [New] `jsx-no-constructed-context-values`: add new rule which checks when the value passed to a Context Provider will cause needless rerenders Adds a new rule that checks if the value prop passed to a Context Provider will cause needless rerenders. A common example is inline object declaration such as: ```js ... ``` which will cause the Context to rerender every time this is run because the object is defined with a new reference. This can lead to performance hits since not only will this rerender all the children of a context, it will also update all consumers of the value. The search React does to search the tree can also be very expensive. Some other instances that this rule covers are: array declarations, function declarations (i.e. arrow functions), or new class object instantians. --- CHANGELOG.md | 3 + README.md | 1 + .../jsx-no-constructed-context-values.md | 37 ++ index.js | 1 + .../jsx-no-constructed-context-values.js | 215 +++++++++ .../jsx-no-constructed-context-values.js | 408 ++++++++++++++++++ 6 files changed, 665 insertions(+) create mode 100644 docs/rules/jsx-no-constructed-context-values.md create mode 100644 lib/rules/jsx-no-constructed-context-values.js create mode 100644 tests/lib/rules/jsx-no-constructed-context-values.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 209e4e9a09..c35cfad5bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,8 +8,10 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Added * [`jsx-key`]: added `checkKeyMustBeforeSpread` option for new jsx transform ([#2835][] @morlay) * [`jsx-newline`]: add new rule ([#2693][] @jzabala) +* [`jsx-no-constructed-context-values`]: add new rule which checks when the value passed to a Context Provider will cause needless rerenders ([#2763][] @dylanOshima) [#2835]: https://github.com/yannickcr/eslint-plugin-react/pull/2835 +[#2763]: https://github.com/yannickcr/eslint-plugin-react/pull/2763 [#2693]: https://github.com/yannickcr/eslint-plugin-react/pull/2693 ## [7.21.5] - 2020.10.19 @@ -3219,3 +3221,4 @@ If you're still not using React 15 you can keep the old behavior by setting the [`no-adjacent-inline-elements`]: docs/rules/no-adjacent-inline-elements.md [`function-component-definition`]: docs/rules/function-component-definition.md [`jsx-newline`]: docs/rules/jsx-newline.md +[`jsx-no-constructed-context-values`]: docs/rules/jsx-no-constructed-context-values.md diff --git a/README.md b/README.md index fa206b90b6..68461e0ca9 100644 --- a/README.md +++ b/README.md @@ -175,6 +175,7 @@ Enable the rules that you would like to use. * [react/jsx-newline](docs/rules/jsx-newline.md): Enforce a new line after jsx elements and expressions (fixable) * [react/jsx-no-bind](docs/rules/jsx-no-bind.md): Prevents usage of Function.prototype.bind and arrow functions in React component props * [react/jsx-no-comment-textnodes](docs/rules/jsx-no-comment-textnodes.md): Comments inside children section of tag should be placed inside braces +* [react/jsx-no-constructed-context-values](docs/rules/jsx-no-constructed-context-values.md): Prevents JSX context provider values from taking values that will cause needless rerenders. * [react/jsx-no-duplicate-props](docs/rules/jsx-no-duplicate-props.md): Enforce no duplicate props * [react/jsx-no-literals](docs/rules/jsx-no-literals.md): Prevent using string literals in React component definition * [react/jsx-no-script-url](docs/rules/jsx-no-script-url.md): Forbid `javascript:` URLs diff --git a/docs/rules/jsx-no-constructed-context-values.md b/docs/rules/jsx-no-constructed-context-values.md new file mode 100644 index 0000000000..c0b79de23e --- /dev/null +++ b/docs/rules/jsx-no-constructed-context-values.md @@ -0,0 +1,37 @@ +# Prevent react contexts from taking non-stable values (react/jsx-no-constructed-context-values) + +This rule prevents non-stable values (i.e. object identities) from being used as a value for `Context.Provider`. + +## Rule Details + +One way to resolve this issue may be to wrap the value in a `useMemo()`. If it's a function then `useCallback()` can be used as well. + +If you _expect_ the context to be rerun on each render, then consider adding a comment/lint supression explaining why. + +## Examples + +Examples of **incorrect** code for this rule: + +``` +return ( + + ... + +) +``` + +Examples of **correct** code for this rule: + +``` +const foo = useMemo(() => {foo: 'bar'}, []); +return ( + + ... + +) +``` + +## Legitimate Uses +React Context, and all its child nodes and Consumers are rerendered whenever the value prop changes. Because each Javascript object carries its own *identity*, things like object expressions (`{foo: 'bar'}`) or function expressions get a new identity on every run through the component. This makes the context think it has gotten a new object and can cause needless rerenders and unintended consequences. + +This can be a pretty large performance hit because not only will it cause the context providers and consumers to rerender with all the elements in its subtree, the processing for the tree scan react does to render the provider and find consumers is also wasted. \ No newline at end of file diff --git a/index.js b/index.js index a533fe3e8f..8edb1177f1 100644 --- a/index.js +++ b/index.js @@ -34,6 +34,7 @@ const allRules = { 'jsx-newline': require('./lib/rules/jsx-newline'), 'jsx-no-bind': require('./lib/rules/jsx-no-bind'), 'jsx-no-comment-textnodes': require('./lib/rules/jsx-no-comment-textnodes'), + 'jsx-no-constructed-context-values': require('./lib/rules/jsx-no-constructed-context-values'), 'jsx-no-duplicate-props': require('./lib/rules/jsx-no-duplicate-props'), 'jsx-no-literals': require('./lib/rules/jsx-no-literals'), 'jsx-no-script-url': require('./lib/rules/jsx-no-script-url'), diff --git a/lib/rules/jsx-no-constructed-context-values.js b/lib/rules/jsx-no-constructed-context-values.js new file mode 100644 index 0000000000..95c370d9b1 --- /dev/null +++ b/lib/rules/jsx-no-constructed-context-values.js @@ -0,0 +1,215 @@ +/** + * @fileoverview Prevents jsx context provider values from taking values that + * will cause needless rerenders. + * @author Dylan Oshima + */ + +'use strict'; + +const docsUrl = require('../util/docsUrl'); + +// ------------------------------------------------------------------------------ +// Helpers +// ------------------------------------------------------------------------------ + +// Recursively checks if an element is a construction. +// A construction is a variable that changes identity every render. +function isConstruction(node, callScope) { + switch (node.type) { + case 'Literal': + if (node.regex != null) { + return {type: 'regular expression', node}; + } + return null; + case 'Identifier': { + const variableScoping = callScope.set.get(node.name); + + if (variableScoping == null || variableScoping.defs == null) { + // If it's not in scope, we don't care. + return null; // Handled + } + + // Gets the last variable identity + const variableDefs = variableScoping.defs; + const def = variableDefs[variableDefs.length - 1]; + if (def != null + && def.type !== 'Variable' + && def.type !== 'FunctionName' + ) { + // Parameter or an unusual pattern. Bail out. + return null; // Unhandled + } + + if (def.node.type === 'FunctionDeclaration') { + return {type: 'function declaration', node: def.node, usage: node}; + } + + const init = def.node.init; + if (init == null) { + return null; + } + + const initConstruction = isConstruction(init, callScope); + if (initConstruction == null) { + return null; + } + + return { + type: initConstruction.type, + node: initConstruction.node, + usage: node + }; + } + case 'ObjectExpression': + // Any object initialized inline will create a new identity + return {type: 'object', node}; + case 'ArrayExpression': + return {type: 'array', node}; + case 'ArrowFunctionExpression': + case 'FunctionExpression': + // Functions that are initialized inline will have a new identity + return {type: 'function expression', node}; + case 'ClassExpression': + return {type: 'class expression', node}; + case 'NewExpression': + // `const a = new SomeClass();` is a construction + return {type: 'new expression', node}; + case 'ConditionalExpression': + return (isConstruction(node.consequent, callScope) + || isConstruction(node.alternate, callScope) + ); + case 'LogicalExpression': + return (isConstruction(node.left, callScope) + || isConstruction(node.right, callScope) + ); + case 'MemberExpression': { + const objConstruction = isConstruction(node.object, callScope); + if (objConstruction == null) { + return null; + } + return { + type: objConstruction.type, + node: objConstruction.node, + usage: node.object + }; + } + case 'JSXFragment': + return {type: 'JSX fragment', node}; + case 'JSXElement': + return {type: 'JSX element', node}; + case 'AssignmentExpression': { + const construct = isConstruction(node.right); + if (construct != null) { + return { + type: 'assignment expression', + node: construct.node, + usage: node + }; + } + return null; + } + case 'TypeCastExpression': + case 'TSAsExpression': + return isConstruction(node.expression); + default: + return null; + } +} + +// ------------------------------------------------------------------------------ +// Rule Definition +// ------------------------------------------------------------------------------ + +module.exports = { + meta: { + docs: { + description: 'Prevents JSX context provider values from taking values that will cause needless rerenders.', + category: 'Best Practices', + recommended: false, + url: docsUrl('jsx-no-constructed-context-values') + }, + messages: { + withIdentifierMsg: + "The '{{variableName}}' {{type}} (at line {{nodeLine}}) passed as the value prop to the Context provider (at line {{usageLine}}) changes every render. To fix this consider wrapping it in a useMemo hook.", + withIdentifierMsgFunc: + "The '{{variableName}}' {{type}} (at line {{nodeLine}}) passed as the value prop to the Context provider (at line {{usageLine}}) changes every render. To fix this consider wrapping it in a useCallback hook.", + defaultMsg: + 'The {{type}} passed as the value prop to the Context provider (at line {{nodeLine}}) changes every render. To fix this consider wrapping it in a useMemo hook.', + defaultMsgFunc: + 'The {{type}} passed as the value prop to the Context provider (at line {{nodeLine}}) changes every render. To fix this consider wrapping it in a useCallback hook.' + } + }, + + create(context) { + return { + JSXOpeningElement(node) { + const openingElementName = node.name; + if (openingElementName.type !== 'JSXMemberExpression') { + // Has no member + return; + } + + const isJsxContext = openingElementName.property.name === 'Provider'; + if (!isJsxContext) { + // Member is not Provider + return; + } + + // Contexts can take in more than just a value prop + // so we need to iterate through all of them + const jsxValueAttribute = node.attributes.find( + (attribute) => attribute.type === 'JSXAttribute' && attribute.name.name === 'value' + ); + + if (jsxValueAttribute == null) { + // No value prop was passed + return; + } + + const valueNode = jsxValueAttribute.value; + if (valueNode.type !== 'JSXExpressionContainer') { + // value could be a literal + return; + } + + const valueExpression = valueNode.expression; + const invocationScope = context.getScope(); + + // Check if the value prop is a construction + const constructInfo = isConstruction(valueExpression, invocationScope); + if (constructInfo == null) { + return; + } + + // Report found error + const constructType = constructInfo.type; + const constructNode = constructInfo.node; + const constructUsage = constructInfo.usage; + const data = { + type: constructType, nodeLine: constructNode.loc.start.line + }; + let messageId = 'defaultMsg'; + + // Variable passed to value prop + if (constructUsage != null) { + messageId = 'withIdentifierMsg'; + data.usageLine = constructUsage.loc.start.line; + data.variableName = constructUsage.name; + } + + // Type of expression + if (constructType === 'function expression' + || constructType === 'function declaration' + ) { + messageId += 'Func'; + } + + context.report({ + node: constructNode, + messageId, + data + }); + } + }; + } +}; diff --git a/tests/lib/rules/jsx-no-constructed-context-values.js b/tests/lib/rules/jsx-no-constructed-context-values.js new file mode 100644 index 0000000000..c199ab7bf0 --- /dev/null +++ b/tests/lib/rules/jsx-no-constructed-context-values.js @@ -0,0 +1,408 @@ +/** + * @fileoverview Prevents jsx context provider values from taking values that + * will cause needless rerenders. + * @author Dylan Oshima + */ + +'use strict'; + +// ----------------------------------------------------------------------------- +// Requirements +// ----------------------------------------------------------------------------- + +const RuleTester = require('eslint').RuleTester; +const rule = require('../../../lib/rules/jsx-no-constructed-context-values'); + +const parsers = require('../../helpers/parsers'); + +const parserOptions = { + ecmaVersion: 2018, + sourceType: 'module', + ecmaFeatures: { + jsx: true + } +}; + +// ----------------------------------------------------------------------------- +// Tests +// ----------------------------------------------------------------------------- + +const ruleTester = new RuleTester({parserOptions}); +ruleTester.run('react-no-constructed-context-values', rule, { + valid: [ + { + code: '' + }, + { + code: '' + }, + { + code: '' + }, + { + code: 'function Component() { const foo = useMemo(() => { return {} }, []); return ()}', + options: [{allowArrowFunctions: true}] + }, + { + code: ` + function Component({oneProp, twoProp, redProp, blueProp,}) { + return ( + + ); + } + ` + }, + { + code: ` + function Foo(section) { + const foo = section.section_components?.edges; + + return ( + + ) + } + `, + parser: parsers.BABEL_ESLINT + }, + { + code: ` + import foo from 'foo'; + function innerContext() { + return ( + + ) + } + ` + }, + { + code: ` + // Passes because the lint rule doesn't handle JSX spread attributes + function innerContext() { + const foo = {value: 'something'} + return ( + + ) + } + ` + }, + { + code: ` + // Passes because the lint rule doesn't handle JSX spread attributes + function innerContext() { + const foo = useMemo(() => { + return bar; + }) + return ( + + ) + } + ` + }, + { + code: ` + // Passes because we can't statically check if it's using the default value + function Component({ a = {} }) { + return (); + } + ` + } + ], + invalid: [ + { + // Invalid because object construction creates a new identity + code: + 'function Component() { const foo = {}; return () }', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because array construction creates a new identity + code: + 'function Component() { const foo = []; return () }', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'array', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because arrow Function creates a new identity + code: + 'function Component() { const foo = () => {}; return ()}', + errors: [{ + messageId: 'withIdentifierMsgFunc', + data: { + variableName: 'foo', + type: 'function expression', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because function expression creates a new identity + code: + 'function Component() { const foo = function bar(){}; return ()}', + errors: [{ + messageId: 'withIdentifierMsgFunc', + data: { + variableName: 'foo', + type: 'function expression', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because class expression creates a new identity + code: + 'function Component() { const foo = class SomeClass{}; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'class expression', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because new expression creates a new identity + code: + 'function Component() { const foo = new SomeClass(); return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'new expression', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // // Invalid because function declaration creates a new identity + code: + 'function Component() { function foo() {}; return ()}', + errors: [{ + messageId: 'withIdentifierMsgFunc', + data: { + variableName: 'foo', + type: 'function declaration', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value of the ternrary will create a new identity + code: + 'function Component() { const foo = true ? {} : "fine"; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value of the logical OR will create a new identity + code: + 'function Component() { const foo = bar || {}; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value of the logical AND will create a new identity + code: + 'function Component() { const foo = bar && {}; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value of the nested ternary will create a new identity + code: + 'function Component() { const foo = bar ? baz ? {} : null : null; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value will create a new identity + code: + 'function Component() { let foo = {}; return () }', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the object value will create a new identity + code: + 'function Component() { var foo = {}; return ()}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'foo', + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Valid, but currently not handled at the moment. + code: ` + function Component() { + let a = {}; + a = 10; + return (); + } + `, + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'a', + type: 'object', + nodeLine: '3', + usageLine: '5' + } + }] + }, + { + // Invalid variable reassignment from parameter because bar is an object identity + code: ` + function Component() { + const foo = {}; + const bar = foo; + return (); + } + `, + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'bar', + type: 'object', + nodeLine: '3', + usageLine: '5' + } + }] + }, + { + // Invalid because the object expression possibly returned from the ternary will create a new identity + code: ` + function Component(foo) { + let bar = true ? foo : {}; + return (); + } + `, + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'bar', + type: 'object', + nodeLine: '3', + usageLine: '4' + } + }], + parser: parsers.BABEL_ESLINT + }, + { + // Invalid because inline object construction will create a new identity + code: 'function Component() { return ();}', + errors: [{ + messageId: 'defaultMsg', + data: { + type: 'object', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because Wrapper returns JSX which has a new identity + code: 'function Component() { const Wrapper = (); return ();}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'Wrapper', + type: 'JSX element', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because RegEx returns a new object which has will be a new identity + code: 'function Component() { const someRegex = /HelloWorld/; return ();}', + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'someRegex', + type: 'regular expression', + nodeLine: '1', + usageLine: '1' + } + }] + }, + { + // Invalid because the right hand side of the assignment expression contains a function which will create a new identity + code: ` + function Component() { + let foo = null; + let bar = x = () => {}; + return (); + } + `, + errors: [{ + messageId: 'withIdentifierMsg', + data: { + variableName: 'bar', + type: 'assignment expression', + nodeLine: '4', + usageLine: '5' + } + }] + } + ] +});