From 4cfe5d20cb3e1e84f0b72edd591dba9eb8d33c70 Mon Sep 17 00:00:00 2001 From: cyan33 Date: Fri, 30 Oct 2020 14:50:56 -0700 Subject: [PATCH] [New] add `no-object-type-as-default-prop` rule Co-authored-by: cyan33 Co-authored-by: fengkx Co-authored-by: Jordan Harband --- CHANGELOG.md | 3 + README.md | 1 + configs/all.js | 1 + docs/rules/no-object-type-as-default-prop.md | 70 +++++++ lib/rules/no-object-type-as-default-prop.js | 108 ++++++++++ .../rules/no-object-type-as-default-prop.js | 188 ++++++++++++++++++ 6 files changed, 371 insertions(+) create mode 100644 docs/rules/no-object-type-as-default-prop.md create mode 100644 lib/rules/no-object-type-as-default-prop.js create mode 100644 tests/lib/rules/no-object-type-as-default-prop.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 51a60eaa52..d28374af5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange * support new config system ([#3429][] @jjangga0214) * [`hook-use-state`]: add `allowDestructuredState` option ([#3449][] @ljharb) * add [`sort-default-props`] and deprecate [`jsx-sort-default-props`] ([#1861][] @alexzherdev) +* add [`no-object-type-as-default-prop`] rule ([#2848][] @cyan33 @fengkx) ### Changed * [Perf] component detection: improve performance by avoiding traversing parents unnecessarily ([#3459][] @golopot) @@ -16,6 +17,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange [#3459]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3459 [#3449]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3449 [#3424]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3429 +[#2848]: https://github.com/jsx-eslint/eslint-plugin-react/pull/2848 [#1861]: https://github.com/jsx-eslint/eslint-plugin-react/pull/1861 ### Fixed @@ -4011,6 +4013,7 @@ If you're still not using React 15 you can keep the old behavior by setting the [`no-is-mounted`]: docs/rules/no-is-mounted.md [`no-multi-comp`]: docs/rules/no-multi-comp.md [`no-namespace`]: docs/rules/no-namespace.md +[`no-object-type-as-default-prop`]: docs/rules/no-object-type-as-default-prop.md [`no-redundant-should-component-update`]: docs/rules/no-redundant-should-component-update.md [`no-render-return-value`]: docs/rules/no-render-return-value.md [`no-set-state`]: docs/rules/no-set-state.md diff --git a/README.md b/README.md index aa80406357..8cae59f3db 100644 --- a/README.md +++ b/README.md @@ -315,6 +315,7 @@ module.exports = [ | ✔ | | | [react/no-is-mounted](docs/rules/no-is-mounted.md) | Disallow usage of isMounted | | | | | [react/no-multi-comp](docs/rules/no-multi-comp.md) | Disallow multiple component definition per file | | | | | [react/no-namespace](docs/rules/no-namespace.md) | Enforce that namespaces are not used in React elements | +| | | | [react/no-object-type-as-default-prop](docs/rules/no-object-type-as-default-prop.md) | Disallow usage of referential-type variables as default param in functional component | | | | | [react/no-redundant-should-component-update](docs/rules/no-redundant-should-component-update.md) | Disallow usage of shouldComponentUpdate when extending React.PureComponent | | ✔ | | | [react/no-render-return-value](docs/rules/no-render-return-value.md) | Disallow usage of the return value of ReactDOM.render | | | | | [react/no-set-state](docs/rules/no-set-state.md) | Disallow usage of setState | diff --git a/configs/all.js b/configs/all.js index e44f2ca52e..e0134738df 100644 --- a/configs/all.js +++ b/configs/all.js @@ -88,6 +88,7 @@ const allRules = { 'no-unused-class-component-methods': require('../lib/rules/no-unused-class-component-methods'), 'no-unused-prop-types': require('../lib/rules/no-unused-prop-types'), 'no-unused-state': require('../lib/rules/no-unused-state'), + 'no-object-type-as-default-prop': require('../lib/rules/no-object-type-as-default-prop'), 'no-will-update-set-state': require('../lib/rules/no-will-update-set-state'), 'prefer-es6-class': require('../lib/rules/prefer-es6-class'), 'prefer-exact-props': require('../lib/rules/prefer-exact-props'), diff --git a/docs/rules/no-object-type-as-default-prop.md b/docs/rules/no-object-type-as-default-prop.md new file mode 100644 index 0000000000..a7bbed518a --- /dev/null +++ b/docs/rules/no-object-type-as-default-prop.md @@ -0,0 +1,70 @@ +# Disallow usage of referential-type variables as default param in functional component (react/no-object-type-as-default-prop) + +💼 This rule is enabled in the following [configs](https://github.com/jsx-eslint/eslint-plugin-react#shareable-configurations): `all`. + +Warns if in a functional component, an object type value (such as array/object literal/function/etc) is used as default prop, to prevent potential unnecessary rerenders, and performance regressions. + +## Rule Details + +Certain values (like arrays, objects, functions, etc) are compared by identity instead of by value. This means that, for example, whilst two empty arrays conceptually represent the same value - JavaScript semantics dictate that they are distinct and unequal as they represent two distinct values. + +When using object destructuring syntax you can set the default value for a given property if it does not exist. If you set the default value to one of the values that is compared by identity, it will mean that each time the destructure is evaluated the JS engine will create a new, distinct value in the destructured variable. + +In the context of a React functional component's props argument this means for each render, the property has a new, distinct value. When this value is passed to a hook as a dependency or passed into a child component as a property React will see this as a new value - meaning that a hook will be re-evaluated, or a memoized component will rerender. + +This obviously destroys any performance benefits you get from memoization. Additionally, in certain circumstances this can cause infinite rerender loops, which can often be hard to debug. + +It's worth noting that primitive literal values (`string`, `number`, `boolean`, `null`, and `undefined`) can be considered to be compared "by value", or alternatively, as always having the same identity (every `3` is the same exact `3`). Thus, it's safe for those to be inlined as a default value. + +To fix the violations, the easiest way is to use a referencing variable in module scope instead of using the literal values, e.g: + +```jsx +const emptyArray = []; + +function Component({ + items = emptyArray, +}) {} +``` + +Examples of ***invalid*** code for this rule: + +```jsx +function Component({ + items = [], +}) {} + +const Component = ({ + items = {}, +}) => {} + +const Component = ({ + items = () => {}, +}) => {} +``` + +Examples of ***valid*** code for this rule: + +```jsx +const emptyArray = []; + +function Component({ + items = emptyArray, +}) {} + +const emptyObject = {}; +const Component = ({ + items = emptyObject, +}) => {} + +const noopFunc = () => {}; +const Component = ({ + items = noopFunc, +}) => {} + +// primitives are all compared by value, so are safe to be inlined +function Component({ + num = 3, + str = 'foo', + bool = true, +}) {} +``` diff --git a/lib/rules/no-object-type-as-default-prop.js b/lib/rules/no-object-type-as-default-prop.js new file mode 100644 index 0000000000..ef8d63a368 --- /dev/null +++ b/lib/rules/no-object-type-as-default-prop.js @@ -0,0 +1,108 @@ +/** + * @fileoverview Prevent usage of referential-type variables as default param in functional component + * @author Chang Yan + */ + +'use strict'; + +const values = require('object.values'); + +const Components = require('../util/Components'); +const docsUrl = require('../util/docsUrl'); +const report = require('../util/report'); + +const FORBIDDEN_TYPES_MAP = { + ArrowFunctionExpression: 'arrow function', + FunctionExpression: 'function expression', + ObjectExpression: 'object literal', + ArrayExpression: 'array literal', + ClassExpression: 'class expression', + NewExpression: 'construction expression', + JSXElement: 'JSX element', +}; + +const FORBIDDEN_TYPES = new Set(Object.keys(FORBIDDEN_TYPES_MAP)); +const MESSAGE_ID = 'forbiddenTypeDefaultParam'; + +const messages = { + [MESSAGE_ID]: '{{propName}} has a/an {{forbiddenType}} as default prop. This could lead to potential infinite render loop in React. Use a variable reference instead of {{forbiddenType}}.', +}; +function hasUsedObjectDestructuringSyntax(params) { + return ( + params != null + && params.length === 1 + && params[0].type === 'ObjectPattern' + ); +} + +function verifyDefaultPropsDestructuring(context, properties) { + // Loop through each of the default params + properties.filter((prop) => prop.type === 'Property').forEach((prop) => { + const propName = prop.key.name; + const propDefaultValue = prop.value; + + if (propDefaultValue.type !== 'AssignmentPattern') { + return; + } + + const propDefaultValueType = propDefaultValue.right.type; + + if ( + propDefaultValueType === 'Literal' + && propDefaultValue.right.regex != null + ) { + report(context, messages[MESSAGE_ID], MESSAGE_ID, { + node: propDefaultValue, + data: { + propName, + forbiddenType: 'regex literal', + }, + }); + } else if ( + propDefaultValueType === 'CallExpression' + && propDefaultValue.right.callee.type === 'Identifier' + && propDefaultValue.right.callee.name === 'Symbol' + ) { + report(context, messages[MESSAGE_ID], MESSAGE_ID, { + node: propDefaultValue, + data: { + propName, + forbiddenType: 'Symbol literal', + }, + }); + } else if (FORBIDDEN_TYPES.has(propDefaultValueType)) { + report(context, messages[MESSAGE_ID], MESSAGE_ID, { + node: propDefaultValue, + data: { + propName, + forbiddenType: FORBIDDEN_TYPES_MAP[propDefaultValueType], + }, + }); + } + }); +} + +module.exports = { + meta: { + docs: { + description: 'Disallow usage of referential-type variables as default param in functional component', + category: 'Best Practices', + recommended: false, + url: docsUrl('no-object-type-as-default-prop'), + }, + messages, + }, + create: Components.detect((context, components) => ({ + 'Program:exit'() { + const list = components.list(); + values(list).forEach((component) => { + const node = component.node; + if (!hasUsedObjectDestructuringSyntax(node.params)) { + return; + } + const properties = node.params[0].properties; + verifyDefaultPropsDestructuring(context, properties); + }); + }, + })), +}; diff --git a/tests/lib/rules/no-object-type-as-default-prop.js b/tests/lib/rules/no-object-type-as-default-prop.js new file mode 100644 index 0000000000..e2660fb938 --- /dev/null +++ b/tests/lib/rules/no-object-type-as-default-prop.js @@ -0,0 +1,188 @@ +/** + * @fileoverview Prevent usage of object type variables as default param in functional component + * @author Chang Yan + */ + +'use strict'; + +// ------------------------------------------------------------------------------ +// Requirements +// ------------------------------------------------------------------------------ + +const RuleTester = require('eslint').RuleTester; +const parsers = require('../../helpers/parsers'); +const rule = require('../../../lib/rules/no-object-type-as-default-prop'); + +const parserOptions = { + ecmaVersion: 2018, + sourceType: 'module', + ecmaFeatures: { + jsx: true, + }, +}; + +// ------------------------------------------------------------------------------ +// Tests +// ------------------------------------------------------------------------------ + +const ruleTester = new RuleTester({ parserOptions }); +const MESSAGE_ID = 'forbiddenTypeDefaultParam'; + +const expectedViolations = [ + { + messageId: MESSAGE_ID, + data: { + propName: 'a', + forbiddenType: 'object literal', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'b', + forbiddenType: 'array literal', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'c', + forbiddenType: 'regex literal', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'd', + forbiddenType: 'arrow function', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'e', + forbiddenType: 'function expression', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'f', + forbiddenType: 'class expression', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'g', + forbiddenType: 'construction expression', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'h', + forbiddenType: 'JSX element', + }, + }, + { + messageId: MESSAGE_ID, + data: { + propName: 'i', + forbiddenType: 'Symbol literal', + }, + }, +]; + +ruleTester.run('no-object-type-as-default-prop', rule, { + valid: parsers.all([].concat( + ` + function Foo({ + bar = emptyFunction, + }) { + return null; + } + `, + ` + function Foo({ + bar = emptyFunction, + ...rest + }) { + return null; + } + `, + ` + function Foo({ + bar = 1, + baz = 'hello', + }) { + return null; + } + `, + ` + function Foo(props) { + return null; + } + `, + ` + function Foo(props) { + return null; + } + + Foo.defaultProps = { + bar: () => {} + } + `, + ` + const Foo = () => { + return null; + }; + `, + ` + const Foo = ({bar = 1}) => { + return null; + }; + `, + ` + export default function NotAComponent({foo = {}}) {} + ` + )), + invalid: parsers.all([].concat( + { + code: ` + function Foo({ + a = {}, + b = ['one', 'two'], + c = /regex/i, + d = () => {}, + e = function() {}, + f = class {}, + g = new Thing(), + h = , + i = Symbol('foo') + }) { + return null; + } + `, + errors: expectedViolations, + }, + { + code: ` + const Foo = ({ + a = {}, + b = ['one', 'two'], + c = /regex/i, + d = () => {}, + e = function() {}, + f = class {}, + g = new Thing(), + h = , + i = Symbol('foo') + }) => { + return null; + } + `, + errors: expectedViolations, + } + )), +});