Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($rootScope, ngRepeat): accomodate objects resulting from Object.create(null) #11028

Closed

Conversation

HandyAndyShortStack
Copy link

Use Object.hasOwnProperty.call when iterating over properties of user-supplied objects.
Allow for objects not inheriting from Object.prototype.

…reate(null)

Use Object.hasOwnProperty.call when iterating over properties of user-supplied objects.
Allow for objects not inheriting from Object.prototype.
@lgalfaso
Copy link
Contributor

If we want to support this, then the patch looks good, but every single time we change a function call to a call there is a performance implication. Given that this change is in code that is very hot, then I would like to know the opinion of other

@HandyAndyShortStack
Copy link
Author

If we decide that using call is too expensive, we should update documentation to make it clear that users are expected to supply a hasOwnProperty property for any object watched in scopes or iterated over by ngRepeat.

@lgalfaso
Copy link
Contributor

Hi @HandyAndyShortStack I think something needs to be done. The main issue is that the two places that this PR modifies are super critical, and a minor change can have a big performance change.
Once again, if objects without a prototype are going to be supported, then this looks like the right fix, I just would like other opinions

@Narretz
Copy link
Contributor

Narretz commented Sep 13, 2015

@petebacondarwin landed a commit which implements this for ngRepeat (7ea2c7f), but the didn't land the change for $rootScope(which was also in #9964.
Afaik, the code in ngRepeat isn't actually that hot, as it is only used for objects. But putting it in the rootScope code is probably a bad idea.

@gkalpak
Copy link
Member

gkalpak commented Sep 14, 2015

@Narretz, I think @petebacondarwin landed it separately in 20fb626.

Closing as duplicate of #9964 (which has already been "taken care of").

@gkalpak gkalpak closed this Sep 14, 2015
@petebacondarwin
Copy link
Contributor

I felt that it was worth the performance impact.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants