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

Add hasOwnProperty checks where appropriate #1189

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Collaborator

For boolean-like objects, I've added hasOwnProperty checks in addition to the existing truthiness check even though for most of these dicts, we leave out false keys instead of setting them explicitly to false.

In DOMPropertyOperations, we don't need to check hasOwnProperty if the property is in isStandardName because (with the exception of getPossibleStandardName) the dicts on DOMProperty will now be consistently populated with every valid attribute.

@sophiebits sophiebits closed this Feb 27, 2014
@sophiebits sophiebits reopened this Feb 27, 2014
@jordwalke
Copy link
Contributor

Don't we usually want:

x in y

Instead of

y.hasOwnProperty(x)

(Unless hasOwnProperty is faster)

@sophiebits
Copy link
Collaborator Author

Most (all?) of these things aren't supposed to have real prototype chains, so it's just a question of whether the property comes from Object.prototype. 'constructor' in {} is true while {}.hasOwnProperty('constructor') is false, so using hasOwnProperty should be more correct for these cases.

For boolean-like objects, I've added hasOwnProperty checks in addition to the existing truthiness check even though for most of these dicts, we leave out false keys instead of setting them explicitly to false.

In DOMPropertyOperations, we don't need to check hasOwnProperty if the property is in isStandardName because (with the exception of getPossibleStandardName) the dicts on DOMProperty will now be consistently populated with every valid attribute.
@@ -83,7 +83,7 @@ var ReactTransitionChildMapping = {
var i;
var childMapping = {};
for (var nextKey in next) {
if (nextKeysPending[nextKey]) {
if (nextKeysPending.hasOwnProperty(nextKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should some of these hasOwnProperty checks inside for-in loops be early continue instead of wrapping in a conditional? Now it is quite easy to still run some code after the conditional but still inside the for in loop, perhaps unintentionally? But I don't know those few places in this diff so well.

zpao added a commit that referenced this pull request May 19, 2014
Add hasOwnProperty checks where appropriate
@zpao
Copy link
Member

zpao commented May 19, 2014

Rebased & merged with 0c6bee0

@zpao zpao closed this May 19, 2014
@@ -128,7 +128,7 @@ var topEventMapping = {
var topListenersIDKey = "_reactListenersID" + String(Math.random()).slice(2);

function getListeningForDocument(mountAt) {
if (mountAt[topListenersIDKey] == null) {
if (!mountAt.hasOwnProperty(topListenersIDKey)) {
Copy link
Member

Choose a reason for hiding this comment

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

From @salier - "This is breaking in IE8, where mountAt is [object DispHTMLDocument]."

We can probably just revert this part.

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.

4 participants