Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] Support Flow type spread #2446

Merged
merged 1 commit into from Oct 16, 2019
Merged

[Fix] Support Flow type spread #2446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rules/prop-types.js
View file
Open in desktop
Expand Up @@ -103,7 +103,7 @@ module.exports = {
return true;
}
// Consider every children as declared
if (propType.children === true || propType.containsSpread || propType.containsIndexers) {
if (propType.children === true || propType.containsUnresolvedSpread || propType.containsIndexers) {
return true;
}
if (propType.acceptedProperties) {
Expand Down
3 changes: 3 additions & 0 deletions lib/util/ast.js
View file
Open in desktop
Expand Up @@ -165,6 +165,9 @@ function getKeyValue(context, node) {
stripQuotes(tokens[0].value)
);
}
if (node.type === 'GenericTypeAnnotation') {
return node.id.name;
}
const key = node.key || node.argument;
return key.type === 'Identifier' ? key.name : key.value;
}
Expand Down
42 changes: 34 additions & 8 deletions lib/util/propTypes.js
View file
Open in desktop
Expand Up @@ -32,13 +32,19 @@ function isSuperTypeParameterPropsDeclaration(node) {
* @param {Object[]} properties Array of properties to iterate.
* @param {Function} fn Function to call on each property, receives property key
and property value. (key, value) => void
* @param {Function} [handleSpreadFn] Function to call on each ObjectTypeSpreadProperty, receives the
argument
*/
function iterateProperties(context, properties, fn) {
function iterateProperties(context, properties, fn, handleSpreadFn) {
if (properties && properties.length && typeof fn === 'function') {
for (let i = 0, j = properties.length; i < j; i++) {
const node = properties[i];
const key = getKeyValue(context, node);

if (node.type === 'ObjectTypeSpreadProperty' && typeof handleSpreadFn === 'function') {
handleSpreadFn(node.argument);
}

const value = node.value;
fn(key, value, node);
}
Expand Down Expand Up @@ -121,30 +127,41 @@ module.exports = function propTypesInstructions(context, components, utils) {
},

ObjectTypeAnnotation(annotation, parentName, seen) {
let containsObjectTypeSpread = false;
let containsUnresolvedObjectTypeSpread = false;
let containsSpread = false;
const containsIndexers = Boolean(annotation.indexers && annotation.indexers.length);
const shapeTypeDefinition = {
type: 'shape',
children: {}
};
iterateProperties(context, annotation.properties, (childKey, childValue, propNode) => {
const fullName = [parentName, childKey].join('.');
if (!childKey && !childValue) {
containsObjectTypeSpread = true;
} else {
if (childKey || childValue) {
const types = buildTypeAnnotationDeclarationTypes(childValue, fullName, seen);
types.fullName = fullName;
types.name = childKey;
types.node = propNode;
types.isRequired = !childValue.optional;
shapeTypeDefinition.children[childKey] = types;
}
},
(spreadNode) => {
const key = getKeyValue(context, spreadNode);
const types = buildTypeAnnotationDeclarationTypes(spreadNode, key, seen);
if (!types.children) {
containsUnresolvedObjectTypeSpread = true;
} else {
Object.assign(shapeTypeDefinition, types.children);
}
containsSpread = true;
});

// Mark if this shape has spread or an indexer. We will know to consider all props from this shape as having propTypes,
// but still have the ability to detect unused children of this shape.
shapeTypeDefinition.containsSpread = containsObjectTypeSpread;
ljharb marked this conversation as resolved.
Show resolved Hide resolved
shapeTypeDefinition.containsUnresolvedSpread = containsUnresolvedObjectTypeSpread;
shapeTypeDefinition.containsIndexers = containsIndexers;
// Deprecated: containsSpread is not used anymore in the codebase, ensure to keep API backward compatibility
shapeTypeDefinition.containsSpread = containsSpread;

return shapeTypeDefinition;
},
Expand Down Expand Up @@ -241,7 +258,7 @@ module.exports = function propTypesInstructions(context, components, utils) {
}

/**
* Marks all props found inside ObjectTypeAnnotaiton as declared.
* Marks all props found inside ObjectTypeAnnotation as declared.
*
* Modifies the declaredProperties object
* @param {ASTNode} propTypes
Expand All @@ -253,7 +270,7 @@ module.exports = function propTypesInstructions(context, components, utils) {

iterateProperties(context, propTypes.properties, (key, value, propNode) => {
if (!value) {
ignorePropsValidation = true;
ignorePropsValidation = ignorePropsValidation || propNode.type !== 'ObjectTypeSpreadProperty';
return;
}

Expand All @@ -263,6 +280,15 @@ module.exports = function propTypesInstructions(context, components, utils) {
types.node = propNode;
types.isRequired = !propNode.optional;
declaredPropTypes[key] = types;
}, (spreadNode) => {
const key = getKeyValue(context, spreadNode);
const spreadAnnotation = getInTypeScope(key);
if (!spreadAnnotation) {
ignorePropsValidation = true;
} else {
const spreadIgnoreValidation = declarePropTypesForObjectTypeAnnotation(spreadAnnotation, declaredPropTypes);
ignorePropsValidation = ignorePropsValidation || spreadIgnoreValidation;
}
});

return ignorePropsValidation;
Expand Down
117 changes: 116 additions & 1 deletion tests/lib/rules/default-props-match-prop-types.js
View file
Open in desktop
Expand Up @@ -733,6 +733,58 @@ ruleTester.run('default-props-match-prop-types', rule, {
].join('\n'),
parser: parsers.BABEL_ESLINT
},
{
code: [
'type DefaultProps1 = {|',
' bar1?: string',
'|};',
'type DefaultProps2 = {|',
' ...DefaultProps1,',
' bar2?: string',
'|};',
'type Props = {',
' foo: string,',
' ...DefaultProps2',
'};',

'function Hello(props: Props) {',
' return <div>Hello {props.foo}</div>;',
'}',

'Hello.defaultProps = {',
' bar1: "bar1",',
' bar2: "bar2",',
'};'
].join('\n'),
parser: parsers.BABEL_ESLINT
},
{
code: [
'type DefaultProps1 = {|',
' bar1?: string',
'|};',
'type DefaultProps2 = {|',
' ...DefaultProps1,',
' bar2?: string',
'|};',
'type Props = {',
' foo: string,',
' ...DefaultProps2',
'};',

'class Hello extends React.Component<Props> {',
' render() {',
' return <div>Hello {props.foo}</div>;',
' }',
'}',

'Hello.defaultProps = {',
' bar1: "bar1",',
' bar2: "bar2",',
'};'
].join('\n'),
parser: parsers.BABEL_ESLINT
},
// don't error when variable is not in scope
{
code: [
Expand Down Expand Up @@ -1460,7 +1512,6 @@ ruleTester.run('default-props-match-prop-types', rule, {
column: 3
}]
},
// Investigate why this test fails. Flow type not finding foo?
{
code: [
'function Hello(props: { foo: string }) {',
Expand Down Expand Up @@ -1590,6 +1641,70 @@ ruleTester.run('default-props-match-prop-types', rule, {
message: 'defaultProp "firstProperty" defined for isRequired propType.'
}
]
},
{
code: [
'type DefaultProps = {',
' baz?: string,',
' bar?: string',
'};',

'type Props = {',
' foo: string,',
' ...DefaultProps',
'}',

'function Hello(props: Props) {',
' return <div>Hello {props.foo}</div>;',
'}',
'Hello.defaultProps = { foo: "foo", frob: "frob", baz: "bar" };'
].join('\n'),
parser: parsers.BABEL_ESLINT,
errors: [
{
message: 'defaultProp "foo" defined for isRequired propType.',
line: 12,
column: 24
},
{
message: 'defaultProp "frob" has no corresponding propTypes declaration.',
line: 12,
column: 36
}
]
},
{
code: [
'type DefaultProps = {',
' baz?: string,',
' bar?: string',
'};',

'type Props = {',
' foo: string,',
' ...DefaultProps',
'}',

'class Hello extends React.Component<Props> {',
' render() {',
' return <div>Hello {props.foo}</div>;',
' }',
'}',
'Hello.defaultProps = { foo: "foo", frob: "frob", baz: "bar" };'
].join('\n'),
parser: parsers.BABEL_ESLINT,
errors: [
{
message: 'defaultProp "foo" defined for isRequired propType.',
line: 14,
column: 24
},
{
message: 'defaultProp "frob" has no corresponding propTypes declaration.',
line: 14,
column: 36
}
]
}
]
});
23 changes: 23 additions & 0 deletions tests/lib/rules/no-unused-prop-types.js
View file
Open in desktop
Expand Up @@ -4619,6 +4619,29 @@ ruleTester.run('no-unused-prop-types', rule, {
errors: [{
message: '\'aProp\' PropType is defined but prop is never used'
}]
}, {
// issue #2138
code: `
type UsedProps = {|
usedProp: number,
|};

type UnusedProps = {|
unusedProp: number,
|};

type Props = {| ...UsedProps, ...UnusedProps |};

function MyComponent({ usedProp, notOne }: Props) {
return <div>{usedProp}</div>;
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: "'unusedProp' PropType is defined but prop is never used",
line: 7,
column: 23
}]
}, {
code: `
type Props = {
Expand Down
65 changes: 65 additions & 0 deletions tests/lib/rules/prop-types.js
View file
Open in desktop
Expand Up @@ -1632,6 +1632,23 @@ ruleTester.run('prop-types', rule, {
'}'
].join('\n'),
parser: parsers.BABEL_ESLINT
}, {
code: [
'type OtherProps = {',
' firstname: string,',
'};',
'type Props = {',
' ...OtherProps,',
' lastname: string',
'};',
'class Hello extends React.Component {',
' props: Props;',
' render () {',
' return <div>Hello {this.props.firstname}</div>;',
' }',
'}'
].join('\n'),
parser: parsers.BABEL_ESLINT
}, {
code: [
'type Person = {',
Expand Down Expand Up @@ -2335,6 +2352,30 @@ ruleTester.run('prop-types', rule, {
`,
parser: parsers.BABEL_ESLINT
},
{
// issue #2138
code: `
type UsedProps = {|
usedProp: number,
|};

type UnusedProps = {|
unusedProp: number,
|};

type Props = {| ...UsedProps, ...UnusedProps |};

function MyComponent({ usedProp }: Props) {
return <div>{usedProp}</div>;
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: "'notOne' is missing in props validation",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly a dumb question, but why is this erroring? notOne doesn't appear anywhere else in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghiculescu it is at line 4791

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moroine I'm not sure I understand. Why is a separate test causing this test to fail? Aren't they each independent tests?

line: 8,
column: 34
}]
},
{
// issue #1259
code: `
Expand Down Expand Up @@ -4728,6 +4769,30 @@ ruleTester.run('prop-types', rule, {
message: '\'initialValues\' is missing in props validation'
}]
},
{
// issue #2138
code: `
type UsedProps = {|
usedProp: number,
|};

type UnusedProps = {|
unusedProp: number,
|};

type Props = {| ...UsedProps, ...UnusedProps |};

function MyComponent({ usedProp, notOne }: Props) {
return <div>{usedProp}</div>;
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: "'notOne' is missing in props validation",
line: 12,
column: 42
}]
},
{
// issue #2298
code: `
Expand Down