Skip to content

Commit

Permalink
Merge pull request #2301 from golopot/fix-cached-props-2
Browse files Browse the repository at this point in the history 
[New] `prop-types`: handle variables defined as props
  • Loading branch information
@ljharb
ljharb committed Jun 13, 2019
2 parents 3ae3a9a + 9a63e19 commit e6b4c33
Show file tree
Hide file tree
Showing 3 changed files with 220 additions and 38 deletions.
87 changes: 84 additions & 3 deletions lib/util/usedPropTypes.js
View file
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);
Object.assign(stack[stack.length - 1], {propVariables, hasBeenWritten: true});
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,10 @@ function isPropTypesUsageByMemberExpression(node, context, utils, checkAsyncSafe
module.exports = function usedPropTypesInstructions(context, components, utils) {
const checkAsyncSafeLifeCycles = versionUtil.testReactVersion(context, '16.3.0');

const propVariables = createPropVariables();
const pushScope = propVariables.pushScope;
const popScope = propVariables.popScope;

/**
* Mark a prop type as used
* @param {ASTNode} node The AST node being marked.
Expand Down Expand Up @@ -261,6 +305,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 +366,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
const propName = ast.getKeyValue(context, properties[k]);

if (propName) {
propVariables.set(propName, parentNames.concat(propName));
usedPropTypes.push({
allNames: parentNames.concat([propName]),
name: propName,
Expand Down Expand Up @@ -371,6 +424,7 @@ module.exports = function usedPropTypesInstructions(context, components, utils)
* FunctionDeclaration, or FunctionExpression
*/
function handleFunctionLikeExpressions(node) {
pushScope();
handleSetStateUpdater(node);
markDestructuredFunctionArgumentsAsUsed(node);
}
Expand All @@ -392,6 +446,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 +459,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 +484,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 +499,12 @@ module.exports = function usedPropTypesInstructions(context, components, utils)

FunctionExpression: handleFunctionLikeExpressions,

'FunctionDeclaration:exit': popScope,

'ArrowFunctionExpression:exit': popScope,

'FunctionExpression:exit': popScope,

JSXSpreadAttribute(node) {
const component = components.get(utils.getParentComponent());
components.set(component ? component.node : node, {
Expand All @@ -439,6 +515,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
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 @@ -4473,27 +4526,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
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

0 comments on commit e6b4c33

Please sign in to comment.