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

[New] prop-types: handle variables defined as props #2301

Merged
merged 4 commits into from Jun 13, 2019
Merged

[New] prop-types: handle variables defined as props #2301

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c06bb1
[New] `prop-types`: handle variables defined as props
golopot Jun 7, 2019
89b8143
Apply suggestion: replace concat([a]) with concat(a)
golopot Jun 12, 2019
3a1a0d1
Apply suggestion: replace mutation with Object.assign
golopot Jun 12, 2019
9a63e19
Immediately destructure out propVariables rather than using it as a n…
golopot Jun 12, 2019
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
85 changes: 82 additions & 3 deletions lib/util/usedPropTypes.js
View file
Open in desktop
Expand Up @@ -15,6 +15,46 @@ const ast = require('./ast');
const LIFE_CYCLE_METHODS = ['componentWillReceiveProps', 'shouldComponentUpdate', 'componentWillUpdate', 'componentDidUpdate'];
const ASYNC_SAFE_LIFE_CYCLE_METHODS = ['getDerivedStateFromProps', 'getSnapshotBeforeUpdate', 'UNSAFE_componentWillReceiveProps', 'UNSAFE_componentWillUpdate'];

function createPropVariables() {
/** @type {Map<string, string[]>} Maps the variable to its definition. `props.a.b` is stored as `['a', 'b']` */
let propVariables = new Map();
let hasBeenWritten = false;
const stack = [{propVariables, hasBeenWritten}];
return {
pushScope() {
// popVariables is not copied until first write.
stack.push({propVariables, hasBeenWritten: false});
},
popScope() {
stack.pop();
propVariables = stack[stack.length - 1].propVariables;
hasBeenWritten = stack[stack.length - 1].hasBeenWritten;
},
/**
* Add a variable name to the current scope
* @param {string} name
* @param {string[]} allNames Example: `props.a.b` should be formatted as `['a', 'b']`
*/
set(name, allNames) {
if (!hasBeenWritten) {
// copy on write
propVariables = new Map(propVariables);
stack[stack.length - 1].propVariables = propVariables;
golopot marked this conversation as resolved.
Show resolved Hide resolved
stack[stack.length - 1].hasBeenWritten = true;
}
return propVariables.set(name, allNames);
},
/**
* Get the definition of a variable.
* @param {string} name
* @returns {string[]} Example: `props.a.b` is represented by `['a', 'b']`
*/
get(name) {
return propVariables.get(name);
}
};
}

/**
* Checks if the string is one of `props`, `nextProps`, or `prevProps`
* @param {string} name The AST node being checked.
Expand Down Expand Up @@ -230,6 +270,8 @@ function isPropTypesUsageByMemberExpression(node, context, utils, checkAsyncSafe
module.exports = function usedPropTypesInstructions(context, components, utils) {
const checkAsyncSafeLifeCycles = versionUtil.testReactVersion(context, '16.3.0');

const propVariables = createPropVariables();

/**
* Mark a prop type as used
* @param {ASTNode} node The AST node being marked.
Expand Down Expand Up @@ -261,6 +303,14 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
node.parent.id.parent = node.parent; // patch for bug in eslint@4 in which ObjectPattern has no parent
markPropTypesAsUsed(node.parent.id, allNames);
}

// const a = props.a
if (
node.parent.type === 'VariableDeclarator' &&
node.parent.id.type === 'Identifier'
) {
propVariables.set(node.parent.id.name, allNames);
}
// Do not mark computed props as used.
type = name !== '__COMPUTED_PROP__' ? 'direct' : null;
}
Expand Down Expand Up @@ -314,6 +364,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
const propName = ast.getKeyValue(context, properties[k]);

if (propName) {
propVariables.set(propName, parentNames.concat([propName]));
golopot marked this conversation as resolved.
Show resolved Hide resolved
usedPropTypes.push({
allNames: parentNames.concat([propName]),
name: propName,
Expand Down Expand Up @@ -371,6 +422,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
* FunctionDeclaration, or FunctionExpression
*/
function handleFunctionLikeExpressions(node) {
propVariables.pushScope();
handleSetStateUpdater(node);
markDestructuredFunctionArgumentsAsUsed(node);
}
Expand All @@ -392,6 +444,11 @@ module.exports = function usedPropTypesInstructions(context, components, utils)

return {
VariableDeclarator(node) {
// let props = this.props
if (isThisDotProps(node.init) && isInClassComponent(utils) && node.id.type === 'Identifier') {
propVariables.set(node.id.name, []);
}

// Only handles destructuring
if (node.id.type !== 'ObjectPattern') {
return;
Expand All @@ -400,14 +457,19 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
// let {props: {firstname}} = this
const propsProperty = node.id.properties.find(property => (
property.key &&
(property.key.name === 'props' || property.key.value === 'props') &&
property.value.type === 'ObjectPattern'
(property.key.name === 'props' || property.key.value === 'props')
));
if (propsProperty && node.init.type === 'ThisExpression') {
if (node.init.type === 'ThisExpression' && propsProperty && propsProperty.value.type === 'ObjectPattern') {
markPropTypesAsUsed(propsProperty.value);
return;
}

// let {props} = this
if (node.init.type === 'ThisExpression' && propsProperty && propsProperty.value.name === 'props') {
propVariables.set('props', []);
return;
}

// let {firstname} = props
if (
isCommonVariableNameForProps(node.init.name) &&
Expand All @@ -420,6 +482,12 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
// let {firstname} = this.props
if (isThisDotProps(node.init) && isInClassComponent(utils)) {
markPropTypesAsUsed(node.id);
return;
}

// let {firstname} = thing, where thing is defined by const thing = this.props.**.*
if (propVariables.get(node.init.name)) {
markPropTypesAsUsed(node, propVariables.get(node.init.name));
}
},

Expand All @@ -429,6 +497,12 @@ module.exports = function usedPropTypesInstructions(context, components, utils)

FunctionExpression: handleFunctionLikeExpressions,

'FunctionDeclaration:exit': propVariables.popScope,

'ArrowFunctionExpression:exit': propVariables.popScope,

'FunctionExpression:exit': propVariables.popScope,
ljharb marked this conversation as resolved.
Show resolved Hide resolved

JSXSpreadAttribute(node) {
const component = components.get(utils.getParentComponent());
components.set(component ? component.node : node, {
Expand All @@ -439,6 +513,11 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
MemberExpression(node) {
if (isPropTypesUsageByMemberExpression(node, context, utils, checkAsyncSafeLifeCycles)) {
markPropTypesAsUsed(node);
return;
}

if (propVariables.get(node.object.name)) {
markPropTypesAsUsed(node, propVariables.get(node.object.name));
}
},

Expand Down
74 changes: 53 additions & 21 deletions tests/lib/rules/no-unused-prop-types.js
View file
Open in desktop
Expand Up @@ -479,6 +479,59 @@ ruleTester.run('no-unused-prop-types', rule, {
'};'
].join('\n'),
parser: parsers.BABEL_ESLINT
}, {
code: `
function Foo({ a }) {
return <>{ a.b }</>
}
Foo.propTypes = {
a: PropTypes.shape({
b: PropType.string,
})
}
`,
options: [{skipShapeProps: false}],
parser: parsers.BABEL_ESLINT
}, {
// Destructured assignment with Shape propTypes with skipShapeProps off issue #816
code: `
class Thing extends React.Component {
static propTypes = {
i18n: PropTypes.shape({
gettext: PropTypes.func,
}),
}

render() {
const { i18n } = this.props;
return (
<p>{i18n.gettext('Some Text')}</p>
);
}
}
`,
parser: parsers.BABEL_ESLINT,
options: [{skipShapeProps: false}]
},
{
code: `
class Thing extends React.Component {
static propTypes = {
a: PropTypes.shape({
b: PropTypes.string,
}),
}

render() {
const { a } = this.props;
return (
<p>{ a.b }</p>
);
}
}
`,
parser: parsers.BABEL_ESLINT,
options: [{skipShapeProps: false}]
}, {
code: [
'var Hello = createReactClass({',
Expand Down Expand Up @@ -4472,27 +4525,6 @@ ruleTester.run('no-unused-prop-types', rule, {
}, {
message: '\'prop2.*\' PropType is defined but prop is never used'
}]
}, {
// Destructured assignment with Shape propTypes with skipShapeProps off issue #816
code: [
'export default class NavigationButton extends React.Component {',
' static propTypes = {',
' route: PropTypes.shape({',
' getBarTintColor: PropTypes.func.isRequired,',
' }).isRequired,',
' };',

' renderTitle() {',
' const { route } = this.props;',
' return <Title tintColor={route.getBarTintColor()}>TITLE</Title>;',
' }',
'}'
].join('\n'),
parser: parsers.BABEL_ESLINT,
options: [{skipShapeProps: false}],
errors: [{
message: '\'route.getBarTintColor\' PropType is defined but prop is never used'
}]
}, {
code: [
// issue #1097
Expand Down
97 changes: 83 additions & 14 deletions tests/lib/rules/prop-types.js
View file
Open in desktop
Expand Up @@ -870,17 +870,6 @@ ruleTester.run('prop-types', rule, {
'};'
].join('\n'),
parser: parsers.BABEL_ESLINT
}, {
// Reassigned props are ignored
code: [
'export class Hello extends Component {',
' render() {',
' const props = this.props;',
' return <div>Hello {props.name.firstname} {props[\'name\'].lastname}</div>',
' }',
'}'
].join('\n'),
parser: parsers.BABEL_ESLINT
}, {
code: [
'export default function FooBar(props) {',
Expand Down Expand Up @@ -2453,6 +2442,82 @@ ruleTester.run('prop-types', rule, {
{message: "'foo' is missing in props validation"},
{message: "'foo.bar' is missing in props validation"}
]
},
{
code: `
function Foo({ a }) {
return <p>{ a.nope }</p>
}

Foo.propTypes = {
a: PropTypes.shape({
_: PropType.string,
})
}
`,
errors: [
{message: "'a.nope' is missing in props validation"}
]
},
{
code: `
function Foo(props) {
const { a } = props
return <p>{ a.nope }</p>
}

Foo.propTypes = {
a: PropTypes.shape({
_: PropType.string,
})
}
`,
errors: [
{message: "'a.nope' is missing in props validation"}
]
},
{
code: `
function Foo(props) {
const a = props.a
return <p>{ a.nope }</p>
}

Foo.propTypes = {
a: PropTypes.shape({
_: PropType.string,
})
}
`,
errors: [
{message: "'a.nope' is missing in props validation"}
]
},
{
code: `
class Foo extends Component {
render() {
const props = this.props
return <div>{props.cat}</div>
}
}
`,
errors: [
{message: "'cat' is missing in props validation"}
]
},
{
code: `
class Foo extends Component {
render() {
const {props} = this
return <div>{props.cat}</div>
}
}
`,
errors: [
{message: "'cat' is missing in props validation"}
]
}, {
code: [
'class Hello extends React.Component {',
Expand Down Expand Up @@ -3364,6 +3429,8 @@ ruleTester.run('prop-types', rule, {
].join('\n'),
errors: [{
message: '\'names\' is missing in props validation'
}, {
message: '\'names.map\' is missing in props validation'
}]
}, {
code: [
Expand Down Expand Up @@ -4521,9 +4588,11 @@ ruleTester.run('prop-types', rule, {
}
`,
parser: parsers.BABEL_ESLINT,
errors: [{
message: '\'a\' is missing in props validation'
}]
errors: [
{message: '\'a\' is missing in props validation'},
{message: '\'a.b\' is missing in props validation'},
{message: '\'a.b.c\' is missing in props validation'}
]
},
{
code: `
Expand Down