Skip to content

Conversation

@zichun
Copy link

@zichun zichun commented Apr 9, 2014

No description provided.

@zichun
Copy link
Author

zichun commented Apr 9, 2014

(first pull request: completed CLA)

Copy link
Contributor

Choose a reason for hiding this comment

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

probName?

Copy link
Author

Choose a reason for hiding this comment

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

gah my bad. fixed

@sophiebits
Copy link
Collaborator

(React assumes that you don't modify Object.prototype and I'm not sure why you would pass something other than a plain object here…)

@zichun
Copy link
Author

zichun commented Apr 9, 2014

it is not documented anywhere (or I must have missed it) that there is such an assumption for React. Also, a previous changelog (http://facebook.github.io/react/blog/2013/07/26/react-v0-4-1.html : •Prevent a potential error in event handling if Object.prototype is extended.) seems to suggest that React would like to support environments where Object.prototype is indeed extended.

My personal take is that this check isn't expensive, and will not potentially be a blocker for any current or future React features. It's a not a bad thing to be defensive

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@syranide
Copy link
Contributor

I think it may be a good idea to at least decide for one or the other, not both. It seems like the rest of the code is written defensively with hasOwnProperty so it makes sense at least.

@sophiebits
Copy link
Collaborator

Note that jQuery doesn't support modifying Object.prototype:

http://contribute.jquery.org/wont-fix/#object-prototype-issues

I'm inclined to do the same.

@sebmarkbage
Copy link
Collaborator

JS as a language and built-in power features are going more and more towards hasOwnProperty checks to be defensive. (Like Object.keys) so I think we should do the same. In this case it doesn't have a cost.

@sebmarkbage
Copy link
Collaborator

Can we get a rebase of this PR and make sure that the Travis CI build passes the build. Thanks.

@sophiebits
Copy link
Collaborator

Closing due to inactivity, but happy to take this if it gets rebased with passing tests.

@sophiebits sophiebits closed this Feb 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants