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

Warn on access of props.key and props.ref #5742

Closed
jimfb opened this issue Dec 29, 2015 · 8 comments · Fixed by #5744
Closed

Warn on access of props.key and props.ref #5742

jimfb opened this issue Dec 29, 2015 · 8 comments · Fixed by #5744

Comments

@jimfb
Copy link
Contributor

jimfb commented Dec 29, 2015

The props key and ref are reserved by React, and used internally. Component authors may attempt to access these properties (thinking they could read the value, as per #5740). We could use Object.defineProperty (in dev mode) to warn if props.key or props.ref is accessed, and point the user to a note/discussion somewhere. That way, users who actually run into the issue will always get a timely message that sends them directly to the relevant explanation/discussion.

This would be a good first bug, if we decide we actually want to do it.

@gaearon
Copy link
Collaborator

gaearon commented Dec 29, 2015

This sounds like a great idea to me!

@sophiebits
Copy link
Collaborator

Yeah this seems reasonable.

@alayek
Copy link

alayek commented Jan 1, 2016

hi, I am interested in working on this; as I am looking forward to contribute. Kindly show me the way. Thanks in advance!

@jimfb
Copy link
Contributor Author

jimfb commented Jan 1, 2016

@alayek Take a look at #5744, that should give you a good start and a good idea of what we would be looking for.

@alayek
Copy link

alayek commented Jan 1, 2016

Thanks @jimfb. Checking it out right away.

@alayek
Copy link

alayek commented Jan 2, 2016

@jimfb I have taken a look at the pull request, and associated code review comments.

It seems the warn-once policy is required here, as implemented in https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/isomorphic/classic/element/ReactElementValidator.js

I am not sure I fully understand how this module achieves this, so I am reading it now.

As per the code review comments in #5744 , I am using the fb.me URLs instead of StackOverflow URLs. But the comments over there also mentioned that the PR might break some test cases. I am not sure about the edge cases here.

I feel like I could use some help on understanding this. Instead of using GitHub issue comments, is it possible to use some chat-platform such as IRC or slack room or Gitter?

@jimfb
Copy link
Contributor Author

jimfb commented Jan 2, 2016

@alayek

  • The module achieves deduplication by setting a boolean if it has already warned.
  • The comments about breaking tests are referring to the failing unit tests. Just run npm run test and npm run lint to verify that everything is working before pushing your changes.
  • There are many places where you can discuss, including the IRC channel and/or reactiflux. There are also github issues (which is what our core team predominantly uses), as well as several other channels. The community is pretty big, but nothing is quite as good as reading through the code and github issues, IMHO.

@ManasJayanth
Copy link
Contributor

Sorry, I was away. #5744 is ready to be reviewed.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants