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
merged 4 commits into from Jun 13, 2019

Conversation

golopot
Copy link
Contributor

@golopot golopot 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;
use(props.a);

// cache a property of props
let {a} = props;
use(a.b)

// 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.

lib/util/usedPropTypes.js Outdated Show resolved Hide resolved
lib/util/usedPropTypes.js Outdated Show resolved Hide resolved
lib/util/usedPropTypes.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

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

@ljharb
Copy link
Member

ljharb commented Jun 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants