From ca595b5a429bff58ed4d3e1bf49618c16d36c46c Mon Sep 17 00:00:00 2001 From: kangax Date: Thu, 2 May 2019 15:50:04 -0400 Subject: [PATCH 1/9] Add requireReadOnlyReactProps rule --- .README/README.md | 1 + .README/rules/require-readonly-react-props.md | 67 +++++++++ src/configs/recommended.json | 3 +- src/index.js | 3 + src/rules/requireReadOnlyReactProps.js | 105 ++++++++++++++ .../assertions/requireReadOnlyReactProps.js | 135 ++++++++++++++++++ tests/rules/index.js | 1 + 7 files changed, 314 insertions(+), 1 deletion(-) create mode 100644 .README/rules/require-readonly-react-props.md create mode 100644 src/rules/requireReadOnlyReactProps.js create mode 100644 tests/rules/assertions/requireReadOnlyReactProps.js diff --git a/.README/README.md b/.README/README.md index 76fe340a..1a6a1547 100644 --- a/.README/README.md +++ b/.README/README.md @@ -62,6 +62,7 @@ npm install eslint babel-eslint eslint-plugin-flowtype --save-dev "comma" ], "flowtype/require-parameter-type": 2, + "flowtype/require-readonly-react-props": 0, "flowtype/require-return-type": [ 2, "always", diff --git a/.README/rules/require-readonly-react-props.md b/.README/rules/require-readonly-react-props.md new file mode 100644 index 00000000..f9bc06c0 --- /dev/null +++ b/.README/rules/require-readonly-react-props.md @@ -0,0 +1,67 @@ +### `require-readonly-react-props` + +This rule validates that React props are marked as $ReadOnly. React props are immutable and modifying them could lead to unexpected results. Marking prop shapes as $ReadOnly avoids these issues. + +The rule tries its best to work with both class and functional components. For class components, it does a fuzzy check for one of "Component", "PureComponent", "React.Component" and "React.PureComponent". It doesn't actually infer that those identifiers resolve to a proper `React.Component` object. + +For example, this will NOT be checked: + +```js +import MyReact from 'react'; +class Foo extends MyReact.Component { } +``` + +As a result, you can safely use other classes without getting warnings from this rule: + +```js +class MyClass extends MySuperClass { } +``` + +React's functional components are hard to detect statically. The way it's done here is by searching for JSX within a function. When present, a function is considered a React component: + +```js +// this gets checked +type Props = { }; +function MyComponent(props: Props) { + return

; +} + +// this doesn't get checked since no JSX is present in a function +type Options = { }; +function SomeHelper(options: Options) { + // ... +} + +// this doesn't get checked since no JSX is present directly in a function +function helper() { return

} +function MyComponent(props: Props) { + return helper(); +} +``` + +The rule only works for locally defined props that are marked with a `$ReadOnly`. It doesn't work with imported props or props marked as read-only via covariant notation: + +```js +// the rule has no way of knowing whether ImportedProps are read-only +import { type ImportedProps } from './somewhere'; +class Foo extends React.Component { } + + +// the rule doesn't check for covariant properties, even though technically this object is entirely read-only +type Props = {| + +foo: string +|}; +class Bar extends React.Component { } +``` + + +```js +{ + "rules": { + "flowtype/require-readonly-react-props": 2 + } +} +``` + + + \ No newline at end of file diff --git a/src/configs/recommended.json b/src/configs/recommended.json index 96fe620a..5f7cdabc 100644 --- a/src/configs/recommended.json +++ b/src/configs/recommended.json @@ -20,6 +20,7 @@ "flowtype/require-parameter-type": 0, "flowtype/require-return-type": 0, "flowtype/require-valid-file-annotation": 0, + "flowtype/require-readonly-react-props": 0, "flowtype/semi": 0, "flowtype/space-after-type-colon": [ 2, @@ -45,4 +46,4 @@ "onlyFilesWithFlowAnnotation": false } } -} +} \ No newline at end of file diff --git a/src/index.js b/src/index.js index b791193f..c182dba0 100644 --- a/src/index.js +++ b/src/index.js @@ -20,6 +20,7 @@ import objectTypeDelimiter from './rules/objectTypeDelimiter'; import requireCompoundTypeAlias from './rules/requireCompoundTypeAlias'; import requireExactType from './rules/requireExactType'; import requireParameterType from './rules/requireParameterType'; +import requireReadOnlyReactProps from './rules/requireReadOnlyReactProps'; import requireReturnType from './rules/requireReturnType'; import requireTypesAtTop from './rules/requireTypesAtTop'; import requireValidFileAnnotation from './rules/requireValidFileAnnotation'; @@ -58,6 +59,7 @@ const rules = { 'require-compound-type-alias': requireCompoundTypeAlias, 'require-exact-type': requireExactType, 'require-parameter-type': requireParameterType, + 'require-readonly-react-props': requireReadOnlyReactProps, 'require-return-type': requireReturnType, 'require-types-at-top': requireTypesAtTop, 'require-valid-file-annotation': requireValidFileAnnotation, @@ -104,6 +106,7 @@ export default { 'require-compound-type-alias': 0, 'require-exact-type': 0, 'require-parameter-type': 0, + 'require-readonly-react-props': 0, 'require-return-type': 0, 'require-variable-type': 0, semi: 0, diff --git a/src/rules/requireReadOnlyReactProps.js b/src/rules/requireReadOnlyReactProps.js new file mode 100644 index 00000000..4774fa56 --- /dev/null +++ b/src/rules/requireReadOnlyReactProps.js @@ -0,0 +1,105 @@ +import _ from 'lodash'; + +const schema = []; + +const reComponentName = /^(Pure)?Component$/; + +const isReactComponent = (node) => { + return ( + + // class Foo extends Component { } + // class Foo extends PureComponent { } + node.superClass.type === 'Identifier' && reComponentName.test(node.superClass.name) || + + // class Foo extends React.Component { } + // class Foo extends React.PureComponent { } + node.superClass.type === 'MemberExpression' && + (node.superClass.object.name === 'React' && reComponentName.test(node.superClass.property.name)) + ); +}; + +const create = (context) => { + + const readOnlyTypes = []; + const reportedFunctionalComponents = []; + + const isReadOnly = (node) => { + return node.superTypeParameters.params[0].id && + node.superTypeParameters.params[0].id.name !== '$ReadOnly' && + !readOnlyTypes.includes(node.superTypeParameters.params[0].id.name); + }; + + // type Props = $ReadOnly<{}> + for (const node of context.getSourceCode().ast.body) { + if (node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly') { + readOnlyTypes.push(node.id.name); + } + } + + return { + + // class components + ClassDeclaration (node) { + if (isReactComponent(node) && isReadOnly(node)) { + context.report({ + message: 'You gotta use $ReadOnly for ' + node.superTypeParameters.params[0].id.name, + node + }); + } else if (node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') { + context.report({ + message: 'You gotta use $ReadOnly for class ' + node.id.name, + node + }); + } + }, + + // functional components + JSXElement (node) { + let currentNode = node; + let identifier; + let typeAnnotation; + + while (currentNode && currentNode.type !== 'FunctionDeclaration') { + currentNode = currentNode.parent; + } + + // functional components can only have 1 param + if (currentNode.params.length !== 1) { + return; + } + + if (currentNode.params[0].type === 'Identifier' && + (typeAnnotation = currentNode.params[0].typeAnnotation)) { + + if ((identifier = typeAnnotation.typeAnnotation.id) && + !readOnlyTypes.includes(identifier.name) && + identifier.name !== '$ReadOnly') { + if (reportedFunctionalComponents.includes(identifier)) { + return; + } + + context.report({ + message: 'You gotta use $ReadOnly for ' + identifier.name, + node + }); + + reportedFunctionalComponents.push(identifier); + + return; + } + + if (typeAnnotation.typeAnnotation.type === 'ObjectTypeAnnotation') { + context.report({ + message: 'You gotta use $ReadOnly for component ' + currentNode.id.name, + node + }); + } + } + } + }; +}; + +export default { + create, + schema +}; diff --git a/tests/rules/assertions/requireReadOnlyReactProps.js b/tests/rules/assertions/requireReadOnlyReactProps.js new file mode 100644 index 00000000..111d4def --- /dev/null +++ b/tests/rules/assertions/requireReadOnlyReactProps.js @@ -0,0 +1,135 @@ +export default { + invalid: [ + + // class components + { + code: 'type Props = { }; class Foo extends React.Component { }', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'type OtherProps = { foo: string }; class Foo extends React.Component { }', + errors: [ + { + message: 'You gotta use $ReadOnly for OtherProps' + } + ] + }, + { + code: 'class Foo extends React.Component<{}> { }', + errors: [ + { + message: 'You gotta use $ReadOnly for class Foo' + } + ] + }, + { + code: 'type Props = { bar: {} }; class Foo extends React.Component { }', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'type Props = { }; class Foo extends Component { }', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'type Props = { }; class Foo extends PureComponent { }', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'class Foo extends React.Component { }', + errors: [ + { + message: 'You gotta use $ReadOnly for UnknownProps' + } + ] + }, + + // functional components + { + code: 'type Props = { }; function Foo(props: Props) { return

}', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'type Props = { }; function Foo(props: Props) { return foo ?

: }', + errors: [ + { + message: 'You gotta use $ReadOnly for Props' + } + ] + }, + { + code: 'function Foo(props: {}) { return

}', + errors: [ + { + message: 'You gotta use $ReadOnly for component Foo' + } + ] + }, + { + code: 'function Foo(props: UnknownProps) { return

}', + errors: [ + { + message: 'You gotta use $ReadOnly for UnknownProps' + } + ] + } + ], + valid: [ + + // class components + { + code: 'class Foo extends React.Component<$ReadOnly<{}>> { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.PureComponent { }' + }, + { + code: 'class Foo extends React.Component<$ReadOnly<{}, State>> { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends React.Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends Component { }' + }, + { + code: 'type Props = $ReadOnly<{}>; class Foo extends PureComponent { }' + }, + { + code: 'type FooType = {}; class Foo extends Bar { }' + }, + + // functional components + { + code: 'type Props = {}; function Foo() { }' + }, + { + code: 'type Props = $ReadOnly<{}>; function Foo(props: Props) { }' + }, + { + code: 'type Props = {}; function Foo(props: OtherProps) { }' + } + ] +}; diff --git a/tests/rules/index.js b/tests/rules/index.js index 63b5defd..d9458f12 100644 --- a/tests/rules/index.js +++ b/tests/rules/index.js @@ -31,6 +31,7 @@ const reportingRules = [ 'require-compound-type-alias', 'require-exact-type', 'require-parameter-type', + 'require-readonly-react-props', 'require-return-type', 'require-types-at-top', 'require-valid-file-annotation', From 132de1eb1162ac6028c0df9a8853788e4358bc80 Mon Sep 17 00:00:00 2001 From: kangax Date: Thu, 2 May 2019 16:05:25 -0400 Subject: [PATCH 2/9] Update messages --- src/rules/requireReadOnlyReactProps.js | 13 +++++------ .../assertions/requireReadOnlyReactProps.js | 22 +++++++++---------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/src/rules/requireReadOnlyReactProps.js b/src/rules/requireReadOnlyReactProps.js index 4774fa56..4807db8e 100644 --- a/src/rules/requireReadOnlyReactProps.js +++ b/src/rules/requireReadOnlyReactProps.js @@ -1,5 +1,3 @@ -import _ from 'lodash'; - const schema = []; const reComponentName = /^(Pure)?Component$/; @@ -19,7 +17,6 @@ const isReactComponent = (node) => { }; const create = (context) => { - const readOnlyTypes = []; const reportedFunctionalComponents = []; @@ -42,12 +39,12 @@ const create = (context) => { ClassDeclaration (node) { if (isReactComponent(node) && isReadOnly(node)) { context.report({ - message: 'You gotta use $ReadOnly for ' + node.superTypeParameters.params[0].id.name, + message: node.superTypeParameters.params[0].id.name + ' must be $ReadOnly', node }); } else if (node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') { context.report({ - message: 'You gotta use $ReadOnly for class ' + node.id.name, + message: node.id.name + ' class props must be $ReadOnly', node }); } @@ -62,7 +59,7 @@ const create = (context) => { while (currentNode && currentNode.type !== 'FunctionDeclaration') { currentNode = currentNode.parent; } - + // functional components can only have 1 param if (currentNode.params.length !== 1) { return; @@ -79,7 +76,7 @@ const create = (context) => { } context.report({ - message: 'You gotta use $ReadOnly for ' + identifier.name, + message: identifier.name + ' must be $ReadOnly', node }); @@ -90,7 +87,7 @@ const create = (context) => { if (typeAnnotation.typeAnnotation.type === 'ObjectTypeAnnotation') { context.report({ - message: 'You gotta use $ReadOnly for component ' + currentNode.id.name, + message: currentNode.id.name + ' component props must be $ReadOnly', node }); } diff --git a/tests/rules/assertions/requireReadOnlyReactProps.js b/tests/rules/assertions/requireReadOnlyReactProps.js index 111d4def..a0b7ddb6 100644 --- a/tests/rules/assertions/requireReadOnlyReactProps.js +++ b/tests/rules/assertions/requireReadOnlyReactProps.js @@ -6,7 +6,7 @@ export default { code: 'type Props = { }; class Foo extends React.Component { }', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -14,7 +14,7 @@ export default { code: 'type OtherProps = { foo: string }; class Foo extends React.Component { }', errors: [ { - message: 'You gotta use $ReadOnly for OtherProps' + message: 'OtherProps must be $ReadOnly' } ] }, @@ -22,7 +22,7 @@ export default { code: 'class Foo extends React.Component<{}> { }', errors: [ { - message: 'You gotta use $ReadOnly for class Foo' + message: 'Foo class props must be $ReadOnly' } ] }, @@ -30,7 +30,7 @@ export default { code: 'type Props = { bar: {} }; class Foo extends React.Component { }', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -38,7 +38,7 @@ export default { code: 'type Props = { }; class Foo extends Component { }', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -46,7 +46,7 @@ export default { code: 'type Props = { }; class Foo extends PureComponent { }', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -54,7 +54,7 @@ export default { code: 'class Foo extends React.Component { }', errors: [ { - message: 'You gotta use $ReadOnly for UnknownProps' + message: 'UnknownProps must be $ReadOnly' } ] }, @@ -64,7 +64,7 @@ export default { code: 'type Props = { }; function Foo(props: Props) { return

}', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -72,7 +72,7 @@ export default { code: 'type Props = { }; function Foo(props: Props) { return foo ?

: }', errors: [ { - message: 'You gotta use $ReadOnly for Props' + message: 'Props must be $ReadOnly' } ] }, @@ -80,7 +80,7 @@ export default { code: 'function Foo(props: {}) { return

}', errors: [ { - message: 'You gotta use $ReadOnly for component Foo' + message: 'Foo component props must be $ReadOnly' } ] }, @@ -88,7 +88,7 @@ export default { code: 'function Foo(props: UnknownProps) { return

}', errors: [ { - message: 'You gotta use $ReadOnly for UnknownProps' + message: 'UnknownProps must be $ReadOnly' } ] } From b19d7c3fe66e3bab811633754828ad1f0b53230c Mon Sep 17 00:00:00 2001 From: kangax Date: Thu, 2 May 2019 16:08:05 -0400 Subject: [PATCH 3/9] Change naming --- ...{requireReadOnlyReactProps.js => requireReadonlyReactProps.js} | 0 ...{requireReadOnlyReactProps.js => requireReadonlyReactProps.js} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename src/rules/{requireReadOnlyReactProps.js => requireReadonlyReactProps.js} (100%) rename tests/rules/assertions/{requireReadOnlyReactProps.js => requireReadonlyReactProps.js} (100%) diff --git a/src/rules/requireReadOnlyReactProps.js b/src/rules/requireReadonlyReactProps.js similarity index 100% rename from src/rules/requireReadOnlyReactProps.js rename to src/rules/requireReadonlyReactProps.js diff --git a/tests/rules/assertions/requireReadOnlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js similarity index 100% rename from tests/rules/assertions/requireReadOnlyReactProps.js rename to tests/rules/assertions/requireReadonlyReactProps.js From 015ab66b0fa29c51bb20302175dbc590ca91c77b Mon Sep 17 00:00:00 2001 From: kangax Date: Thu, 2 May 2019 16:11:13 -0400 Subject: [PATCH 4/9] Fix imports --- src/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/index.js b/src/index.js index c182dba0..aed03f9f 100644 --- a/src/index.js +++ b/src/index.js @@ -20,7 +20,7 @@ import objectTypeDelimiter from './rules/objectTypeDelimiter'; import requireCompoundTypeAlias from './rules/requireCompoundTypeAlias'; import requireExactType from './rules/requireExactType'; import requireParameterType from './rules/requireParameterType'; -import requireReadOnlyReactProps from './rules/requireReadOnlyReactProps'; +import requireReadonlyReactProps from './rules/requireReadonlyReactProps'; import requireReturnType from './rules/requireReturnType'; import requireTypesAtTop from './rules/requireTypesAtTop'; import requireValidFileAnnotation from './rules/requireValidFileAnnotation'; @@ -59,7 +59,7 @@ const rules = { 'require-compound-type-alias': requireCompoundTypeAlias, 'require-exact-type': requireExactType, 'require-parameter-type': requireParameterType, - 'require-readonly-react-props': requireReadOnlyReactProps, + 'require-readonly-react-props': requireReadonlyReactProps, 'require-return-type': requireReturnType, 'require-types-at-top': requireTypesAtTop, 'require-valid-file-annotation': requireValidFileAnnotation, From e3d4097c84ecbd09a1829dddb5dc0bb5f704c0fe Mon Sep 17 00:00:00 2001 From: kangax Date: Thu, 2 May 2019 16:12:34 -0400 Subject: [PATCH 5/9] Fix linter --- src/rules/requireReadonlyReactProps.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js index 4807db8e..efd922da 100644 --- a/src/rules/requireReadonlyReactProps.js +++ b/src/rules/requireReadonlyReactProps.js @@ -67,7 +67,6 @@ const create = (context) => { if (currentNode.params[0].type === 'Identifier' && (typeAnnotation = currentNode.params[0].typeAnnotation)) { - if ((identifier = typeAnnotation.typeAnnotation.id) && !readOnlyTypes.includes(identifier.name) && identifier.name !== '$ReadOnly') { From 0a970ff2e0cbaf903a883a1c18f135fc47e40c92 Mon Sep 17 00:00:00 2001 From: kangax Date: Fri, 3 May 2019 12:09:40 -0400 Subject: [PATCH 6/9] Fix some edge cases --- src/rules/requireReadonlyReactProps.js | 8 ++++++-- tests/rules/assertions/requireReadonlyReactProps.js | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js index efd922da..18eeec61 100644 --- a/src/rules/requireReadonlyReactProps.js +++ b/src/rules/requireReadonlyReactProps.js @@ -3,6 +3,10 @@ const schema = []; const reComponentName = /^(Pure)?Component$/; const isReactComponent = (node) => { + if (!node.superClass) { + return false; + } + return ( // class Foo extends Component { } @@ -42,7 +46,7 @@ const create = (context) => { message: node.superTypeParameters.params[0].id.name + ' must be $ReadOnly', node }); - } else if (node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') { + } else if (node.superTypeParameters && node.superTypeParameters.params[0].type === 'ObjectTypeAnnotation') { context.report({ message: node.id.name + ' class props must be $ReadOnly', node @@ -61,7 +65,7 @@ const create = (context) => { } // functional components can only have 1 param - if (currentNode.params.length !== 1) { + if (!currentNode || currentNode.params.length !== 1) { return; } diff --git a/tests/rules/assertions/requireReadonlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js index a0b7ddb6..2d88cf58 100644 --- a/tests/rules/assertions/requireReadonlyReactProps.js +++ b/tests/rules/assertions/requireReadonlyReactProps.js @@ -120,6 +120,9 @@ export default { { code: 'type FooType = {}; class Foo extends Bar { }' }, + { + code: 'class Foo { }' + }, // functional components { @@ -130,6 +133,9 @@ export default { }, { code: 'type Props = {}; function Foo(props: OtherProps) { }' + }, + { + code: 'function Foo() { return

}' } ] }; From 4305b904138de6d6e7ffb57b0d84f8ded81c40aa Mon Sep 17 00:00:00 2001 From: kangax Date: Fri, 3 May 2019 13:26:55 -0400 Subject: [PATCH 7/9] Fix few more edge cases --- src/rules/requireReadonlyReactProps.js | 19 ++++++++++++++----- .../assertions/requireReadonlyReactProps.js | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js index 18eeec61..25023150 100644 --- a/src/rules/requireReadonlyReactProps.js +++ b/src/rules/requireReadonlyReactProps.js @@ -24,16 +24,25 @@ const create = (context) => { const readOnlyTypes = []; const reportedFunctionalComponents = []; - const isReadOnly = (node) => { + const isReadOnlyClassProp = (node) => { return node.superTypeParameters.params[0].id && node.superTypeParameters.params[0].id.name !== '$ReadOnly' && !readOnlyTypes.includes(node.superTypeParameters.params[0].id.name); }; - // type Props = $ReadOnly<{}> + const isReadOnlyType = (node) => { + return node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly'; + }; + for (const node of context.getSourceCode().ast.body) { - if (node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly') { - readOnlyTypes.push(node.id.name); + // type Props = $ReadOnly<{}> + if (isReadOnlyType(node) || + + // export type Props = $ReadOnly<{}> + node.type === 'ExportNamedDeclaration' && + node.declaration && + isReadOnlyType(node.declaration)) { + readOnlyTypes.push(node.id ? node.id.name : node.declaration.id.name); } } @@ -41,7 +50,7 @@ const create = (context) => { // class components ClassDeclaration (node) { - if (isReactComponent(node) && isReadOnly(node)) { + if (isReactComponent(node) && isReadOnlyClassProp(node)) { context.report({ message: node.superTypeParameters.params[0].id.name + ' must be $ReadOnly', node diff --git a/tests/rules/assertions/requireReadonlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js index 2d88cf58..85d2ea4d 100644 --- a/tests/rules/assertions/requireReadonlyReactProps.js +++ b/tests/rules/assertions/requireReadonlyReactProps.js @@ -58,6 +58,14 @@ export default { } ] }, + { + code: 'export type Props = {}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, // functional components { @@ -91,6 +99,14 @@ export default { message: 'UnknownProps must be $ReadOnly' } ] + }, + { + code: 'export type Props = {}; function Foo(props: Props) { return

}', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] } ], valid: [ @@ -123,6 +139,9 @@ export default { { code: 'class Foo { }' }, + { + code: 'export type Props = $ReadOnly<{}>; class Foo extends Component { }' + }, // functional components { From 07eb6e4617550f7d8c1ad1b01b80c1984015434e Mon Sep 17 00:00:00 2001 From: kangax Date: Tue, 7 May 2019 13:43:37 -0400 Subject: [PATCH 8/9] Make covariant notation work --- .README/rules/require-readonly-react-props.md | 16 ++++++++- src/rules/requireReadonlyReactProps.js | 12 ++++++- .../assertions/requireReadonlyReactProps.js | 35 ++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/.README/rules/require-readonly-react-props.md b/.README/rules/require-readonly-react-props.md index f9bc06c0..c8789534 100644 --- a/.README/rules/require-readonly-react-props.md +++ b/.README/rules/require-readonly-react-props.md @@ -47,11 +47,25 @@ import { type ImportedProps } from './somewhere'; class Foo extends React.Component { } -// the rule doesn't check for covariant properties, even though technically this object is entirely read-only +// the rule also checks for covariant properties type Props = {| +foo: string |}; class Bar extends React.Component { } + +// this fails because the object is not fully ReadOnly +type Props = {| + +foo: string, + bar: number, +|}; +class Bar extends React.Component { } + +// this also fails because spreading makes object mutable (as of Flow 0.98) +type Props = {| + +foo: string, + ...bar, +|}; +class Bar extends React.Component { } ``` diff --git a/src/rules/requireReadonlyReactProps.js b/src/rules/requireReadonlyReactProps.js index 25023150..c40bdfd0 100644 --- a/src/rules/requireReadonlyReactProps.js +++ b/src/rules/requireReadonlyReactProps.js @@ -30,8 +30,18 @@ const create = (context) => { !readOnlyTypes.includes(node.superTypeParameters.params[0].id.name); }; + const isReadOnlyObjectType = (node) => { + return node.type === 'TypeAlias' && + node.right && + node.right.type === 'ObjectTypeAnnotation' && + node.right.properties.length > 0 && + node.right.properties.every((prop) => { + return prop.variance && prop.variance.kind === 'plus'; + }); + }; + const isReadOnlyType = (node) => { - return node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly'; + return node.type === 'TypeAlias' && node.right.id && node.right.id.name === '$ReadOnly' || isReadOnlyObjectType(node); }; for (const node of context.getSourceCode().ast.body) { diff --git a/tests/rules/assertions/requireReadonlyReactProps.js b/tests/rules/assertions/requireReadonlyReactProps.js index 85d2ea4d..9cd62284 100644 --- a/tests/rules/assertions/requireReadonlyReactProps.js +++ b/tests/rules/assertions/requireReadonlyReactProps.js @@ -66,6 +66,30 @@ export default { } ] }, + { + code: 'type Props = {| foo: string |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = {| +foo: string, ...bar |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, + { + code: 'type Props = {| +foo: string, -bar: number |}; class Foo extends Component { }', + errors: [ + { + message: 'Props must be $ReadOnly' + } + ] + }, // functional components { @@ -140,7 +164,16 @@ export default { code: 'class Foo { }' }, { - code: 'export type Props = $ReadOnly<{}>; class Foo extends Component { }' + code: 'export type Props = $ReadOnly<{}>; class Foo extends Component { }' + }, + { + code: 'export type Props = $ReadOnly<{}>; export class Foo extends Component { }' + }, + { + code: 'type Props = {| +foo: string |}; class Foo extends Component { }' + }, + { + code: 'type Props = {| +foo: string, +bar: number |}; class Foo extends Component { }' }, // functional components From 09f966280e7253cc00bef0b9ef3ae4ff2e0a8fcb Mon Sep 17 00:00:00 2001 From: kangax Date: Tue, 7 May 2019 13:48:26 -0400 Subject: [PATCH 9/9] Update docs --- .README/rules/require-readonly-react-props.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.README/rules/require-readonly-react-props.md b/.README/rules/require-readonly-react-props.md index c8789534..5ac1d62e 100644 --- a/.README/rules/require-readonly-react-props.md +++ b/.README/rules/require-readonly-react-props.md @@ -39,7 +39,7 @@ function MyComponent(props: Props) { } ``` -The rule only works for locally defined props that are marked with a `$ReadOnly`. It doesn't work with imported props or props marked as read-only via covariant notation: +The rule only works for locally defined props that are marked with a `$ReadOnly` or using covariant notation. It doesn't work with imported props: ```js // the rule has no way of knowing whether ImportedProps are read-only @@ -53,14 +53,15 @@ type Props = {| |}; class Bar extends React.Component { } -// this fails because the object is not fully ReadOnly +// this fails because the object is not fully read-only type Props = {| +foo: string, bar: number, |}; class Bar extends React.Component { } -// this also fails because spreading makes object mutable (as of Flow 0.98) +// this fails because spreading makes object mutable (as of Flow 0.98) +// https://github.com/gajus/eslint-plugin-flowtype/pull/400#issuecomment-489813899 type Props = {| +foo: string, ...bar,