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.

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

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

merged 4 commits into from Jun 13, 2019


None yet
3 participants
Copy link

commented Jun 7, 2019

Fixes #442, fixes #1002, fixes #1257, fixes #1764. (let {props} = this)
Fixes #833, fixes #1116 (const Foo = ({ a }) => <>{ a.b }</>)

This pr fixed cases where a variable is defined as props.**.* For example:

// cache props
let props = this.props;

// cache a property of props
let {a} = props;

// param destructuring
const Foo = ({ a }) => <>{ a.b }</>

In order to tell weather foo.a is a props usage, we need to tell whether foo is defined as props.**.*. For that purpose a Map is used.

Show resolved Hide resolved lib/util/usedPropTypes.js Outdated
Show resolved Hide resolved lib/util/usedPropTypes.js Outdated
Show resolved Hide resolved lib/util/usedPropTypes.js Outdated

golopot and others added some commits Jun 12, 2019

Apply suggestion: replace concat([a]) with concat(a)
Co-Authored-By: Jordan Harband <>
Apply suggestion: replace mutation with Object.assign
Co-Authored-By: Jordan Harband <>

ljharb approved these changes Jun 12, 2019

@ljharb ljharb requested review from yannickcr, EvHaus and lencioni Jun 12, 2019


EvHaus approved these changes Jun 13, 2019

Copy link

left a comment

Nice. Thanks for taking this task on, it's been on my wish list for a very long time. This likely resolved more issues than just the ones you tagged, so we'll want to do a full review all open no-unused-prop-types issues and resolve the remaining ones as well. I can help with that once this lands.

Have you done any testing to ensure that double reassignments will still work? ie:

let {a} = props
let b = a;

return <Used thing={b} />

At the very least, that shouldn't throw new any warnings. Might be good to add a test case for that to confirm.

Otherwise LGTM


This comment has been minimized.

Copy link

commented Jun 13, 2019

I'll merge now; we can add more test cases in a followup.

@ljharb ljharb merged commit e6b4c33 into yannickcr:master Jun 13, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
coverage/coveralls Coverage decreased (-0.004%) to 97.581%
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.