Skip to content

Support read-only props in Flow for no-unused-prop-types #1390

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/rules/no-unused-prop-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,15 +258,23 @@ module.exports = {
return tokens.length && tokens[0].value === '...';
}

/**
* Removes quotes from around an identifier.
* @param {string} the identifier to strip
*/
function stripQuotes(string) {
return string.replace(/^\'|\'$/g, '');
}

/**
* Retrieve the name of a key node
* @param {ASTNode} node The AST node with the key.
* @return {string} the name of the key
*/
function getKeyValue(node) {
if (node.type === 'ObjectTypeProperty') {
const tokens = context.getFirstTokens(node, 1);
return tokens[0].value;
const tokens = context.getFirstTokens(node, {count: 1, filter: tokenNode => ['Identifier', 'String'].indexOf(tokenNode.type) >= 0});
return stripQuotes(tokens[0].value);
}
const key = node.key || node.argument;
return key.type === 'Identifier' ? key.name : key.value;
Expand Down
12 changes: 3 additions & 9 deletions lib/rules/prop-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,7 @@ module.exports = {
* @param {string} the identifier to strip
*/
function stripQuotes(string) {
Copy link
Member

Choose a reason for hiding this comment

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

can this repeated function be extracted out into a shared file?

Copy link
Contributor Author

@jseminck jseminck Aug 23, 2017

Choose a reason for hiding this comment

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

Yeah - that's what I mentioned in the other PR: prop-types and no-unused-prop-types on first sight seem to share quite a bit of logic that deals with extracting the declared prop types for a component. And currently, the logic is not in sync (e.g. props in TypedArgument for Flow is not supported at all for no-unused-prop-types.

I'll try to deal with extracting as much of the shared logic as possible this weekend. Created an issue: #1393

if (string[0] === '\'' || string[0] === '"' && string[0] === string[string.length - 1]) {
return string.slice(1, string.length - 1);
}
return string;
return string.replace(/^\'|\'$/g, '');
}

/**
Expand All @@ -346,11 +343,8 @@ module.exports = {
*/
function getKeyValue(node) {
if (node.type === 'ObjectTypeProperty') {
const tokens = context.getFirstTokens(node, 2);
return (tokens[0].value === '+' || tokens[0].value === '-'
? tokens[1].value
: stripQuotes(tokens[0].value)
);
const tokens = context.getFirstTokens(node, {count: 1, filter: tokenNode => ['Identifier', 'String'].indexOf(tokenNode.type) >= 0});
Copy link
Member

Choose a reason for hiding this comment

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

why would we want to strip all prefixes? only + and - exist, as far as I know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems a much safer implementation for the future. If Flow would introduce a new prefix, it should be handled automatically. (assuming that the prefix token type is similar to the types it has now).

If you think keeping a list of prefixes is better then we can go with that approach as well...

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have explicit support; I'd rather it not silently risk doing the wrong thing when an unanticipated prefix exists.

I definitely don't want the plugin to crash when a new prefix is found - but i might want it to warn on the prop.

return stripQuotes(tokens[0].value);
}
const key = node.key || node.argument;
return key.type === 'Identifier' ? key.name : key.value;
Expand Down
59 changes: 41 additions & 18 deletions tests/lib/rules/no-unused-prop-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -2032,27 +2032,50 @@ ruleTester.run('no-unused-prop-types', rule, {
}, {
// issue #106
code: `
import React from 'react';
import SharedPropTypes from './SharedPropTypes';
import React from 'react';
import SharedPropTypes from './SharedPropTypes';

export default class A extends React.Component {
render() {
return (
<span
a={this.props.a}
b={this.props.b}
c={this.props.c}>
{this.props.children}
</span>
);
export default class A extends React.Component {
render() {
return (
<span
a={this.props.a}
b={this.props.b}
c={this.props.c}>
{this.props.children}
</span>
);
}
}
}

A.propTypes = {
a: React.PropTypes.string,
...SharedPropTypes // eslint-disable-line object-shorthand
};
`,
A.propTypes = {
a: React.PropTypes.string,
...SharedPropTypes // eslint-disable-line object-shorthand
};
`,
parser: 'babel-eslint'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reformatted this test - that's why it's in the PR...

}, {
// issue #933
code: `
type Props = {
+foo: number
}
class MyComponent extends React.Component {
render() {
return <div>{this.props.foo}</div>
}
}
`,
parser: 'babel-eslint'
}, {
code: `
type Props = {
\'completed?\': boolean,
};
const Hello = (props: Props): React.Element => {
return <div>{props[\'completed?\']}</div>;
}
`,
parser: 'babel-eslint'
}
],
Expand Down