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

Warns on access of `props.key` and `props.ref` #5744

Merged

Conversation

Projects
None yet
6 participants
@prometheansacrifice
Copy link
Contributor

commented Dec 29, 2015

Attempt for fix 5742

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2015

Link to SO could be change to a note in the docs

@prometheansacrifice prometheansacrifice changed the title Warns on access of `props.key` and `props.ref` [WIP] Warns on access of `props.key` and `props.ref` Dec 29, 2015

'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (http://stackoverflow.com/questions/33682774/how-to-access-the' +
'-key-property-from-a-reactjs-component)',

This comment has been minimized.

Copy link
@yangshun

yangshun Dec 29, 2015

Member

This link explains about accessing key and not ref. Might be confusing to the reader.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Dec 29, 2015

Author Contributor

Right. The idea is to replace it with a link to relevant documentation being discussed at #5740

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Dec 29, 2015

Author Contributor

This PR needs more work too. Its failing some tests

This comment has been minimized.

Copy link
@jimfb

jimfb Dec 30, 2015

Contributor

We generally use fb.me urls that point to gists. I created one for you to use: https://fb.me/react-special-props

'%s: `key` is not a prop. Trying to access it will result ' +
'in `undefined` being returned. If you need to access the same ' +
'value within the child component, you should pass it as a different ' +
'prop. (http://stackoverflow.com/questions/33682774/how-to-access-the' +

This comment has been minimized.

Copy link
@jimfb
});
expect(console.error.calls.length).toBe(0);
ReactDOM.render(<Parent />, container);
expect(console.error.calls.length).toBe(3);

This comment has been minimized.

Copy link
@jimfb

jimfb Dec 30, 2015

Contributor

We probably want to deduplicate, such that the warning only appears once. Otherwise, the user's dev console is going to be filled with this warning.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Dec 30, 2015

Author Contributor

@jimfb But accessor methods will get triggered for every access. Are you suggesting an alternative to the entire accessor approach?

This comment has been minimized.

Copy link
@austindebruyn

austindebruyn Dec 31, 2015

Check a file like ReactElementValidator to see how a module-global object (ownerHasKeyUseWarning) is used to make sure the warning is printed only once for each offense.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-if-user-accesses-key-ref-props branch from 63a3ea8 to f861879 Jan 18, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jan 18, 2016

@prometheansacrifice updated the pull request.

@prometheansacrifice prometheansacrifice changed the title [WIP] Warns on access of `props.key` and `props.ref` Warns on access of `props.key` and `props.ref` Jan 18, 2016

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2016

@jimfb This PR is ready to be reviewed. Can you please have a look?

'prop. (https://fb.me/react-special-props)',
'displayName' in type ? type.displayName: 'Element'
);
}

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 29, 2016

Contributor

Let's explicitly add a return statement here. This is a getter, so we should return a value, and it makes the expected behavior a little more clear. Same with the getter for ref.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Jan 29, 2016

Author Contributor

Right. I'll wait for @spicyj's comment on the next line note and update this PR afterwards.

ref = config.ref === undefined ? null : config.ref;
key = config.key === undefined ? null : '' + config.key;
ref = !config.hasOwnProperty('ref') ||
Object.getOwnPropertyDescriptor(config, 'ref').get ? null : config.ref;

This comment has been minimized.

Copy link
@jimfb

jimfb Jan 29, 2016

Contributor

It feels unfortunate that this runs in production, @spicyj do we care?

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Jan 29, 2016

Author Contributor

I guess it can be put in a __DEV__ block.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Jan 29, 2016

Collaborator

This should definitely not run in prod. This is a pretty weird condition to begin with though. I don't have anything better off the top of my head but it is weird that all getters here will behave differently.

Also, this is a behavior change if config.ref is set to undefined (null before, now undefined).

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Jan 29, 2016

Author Contributor

The behaviour change can be handled with another condition (check for undefined), however making the checks more ugly.

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 2, 2016

Contributor

Let's just wrap this in a DEV block for now.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 2, 2016

@prometheansacrifice updated the pull request.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

@prometheansacrifice Can you add an explicit return statement as per above, and then I think this is ready to go.

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2016

@jimfb Sure

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Don't forget to add it to both getters (key+ref), even though I just called out one of them.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-if-user-accesses-key-ref-props branch from 55607a8 to c3980a6 Feb 16, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 16, 2016

@prometheansacrifice updated the pull request.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

This looks great, thanks @prometheansacrifice!

jimfb added a commit that referenced this pull request Feb 16, 2016

Merge pull request #5744 from prometheansacrifice/warn-if-user-access…
…es-key-ref-props

Warns on access of `props.key` and `props.ref`

@jimfb jimfb merged commit 3bee2d9 into facebook:master Feb 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@npbee npbee referenced this pull request Apr 2, 2016

Closed

React 15 support #8

@jakeboone02 jakeboone02 referenced this pull request Apr 9, 2016

Merged

[IconMenu] Removed props.ref call #3913

3 of 3 tasks complete

und3fined pushed a commit to und3fined/material-ui that referenced this pull request Apr 20, 2016

[IconMenu] Removed props.ref call
Calls to props.ref will throw a warning in React v15.  This commit hard-codes the
value instead of reading props.ref.  The iconButtonRef element has been removed
from the state object, since it never changes after the component is initialized.

React warnings discussed in this PR:
facebook/react#5744

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
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.