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

fix($compile): use createMap() for $$observe listeners when initialized ... #10446

Closed
wants to merge 1 commit into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Dec 14, 2014

...from attr interpolation

This alternate initialization of $$observers was missed in a27d827.

@googlebot
Copy link

CLAs look good, thanks!

@lgalfaso
Copy link
Contributor

It is my understanding that we are going to use objects without prototype
only if they are not accessible by developers (even for private properties)

@jbedard
Copy link
Contributor Author

jbedard commented Dec 14, 2014

@lgalfaso $$observers already uses createMap, its just this alternate attr-interpolation case that still initializes it as a plain object.

The test failure in IE is interesting though...

@lgalfaso
Copy link
Contributor

mmm... it looks like this was relax some time ago. Then it LGTM

@lgalfaso lgalfaso added this to the 1.3.7 milestone Dec 14, 2014
@lgalfaso lgalfaso self-assigned this Dec 14, 2014
@lgalfaso
Copy link
Contributor

landed as 8e28bb4

@lgalfaso lgalfaso closed this Dec 14, 2014
@pkozlowski-opensource
Copy link
Member

@jbedard @lgalfaso seems like this change upsets IE: https://travis-ci.org/angular/angular.js/jobs/43993200

It looks to me as a legitimate failure so we need to either roll-back or fix this one.

@lgalfaso
Copy link
Contributor

@pkozlowski-opensource you are right, reverting this now

@lgalfaso lgalfaso reopened this Dec 14, 2014
@lgalfaso
Copy link
Contributor

reverted with f389f86

@pkozlowski-opensource
Copy link
Member

Cool - just a minor nit - I think that the commit message for revert should be revert(..): ...

@lgalfaso
Copy link
Contributor

@pkozlowski-opensource done

@lgalfaso lgalfaso modified the milestones: 1.3.x, 1.3.7 Dec 14, 2014
@lgalfaso
Copy link
Contributor

this is not ready to be merged (at least, until IE is happy with it), moving it to 1.3.x

@caitp
Copy link
Contributor

caitp commented Dec 14, 2014

@lgalfaso you're shadowing properties from the prototype chain --- you probably need to change attrs.hasOwnProperty(key) to hasOwnProperty.call(attrs, key) --- but this won't help performance

Also, valueOf and toString are really stupid properties to shadow as non-functions for any object, because they will cause '' + object to throw

@jbedard jbedard force-pushed the compile-attr-obs-createmap branch from 57705c7 to 263a7f6 Compare December 14, 2014 18:47
@jbedard
Copy link
Contributor Author

jbedard commented Dec 14, 2014

I don't understand why only IE failed. But I think it makes sense to not support hasOwnProperty being overridden so I've removed it from the test. This commit wasn't trying to add support for anything but just fix the case where $$observers was initialized in this alternate path.

@caitp
Copy link
Contributor

caitp commented Dec 14, 2014

@jbedard --- I think the reason is that String/Number/Boolean host objects in IE probably don't support hasOwnProperty(), but do in other browsers. Edit: nahhh that's not it. I dunno =]

@jbedard
Copy link
Contributor Author

jbedard commented Dec 14, 2014

The test was causing attrs.hasOwnProperty(key) to basically be "2"(key) which I'd think would fail in every browser. I dunno either :|

@caitp
Copy link
Contributor

caitp commented Dec 14, 2014

Yeah exactly, it's weird.

@jbedard jbedard force-pushed the compile-attr-obs-createmap branch from 263a7f6 to c6caf7d Compare December 16, 2014 06:29
@jbedard
Copy link
Contributor Author

jbedard commented Dec 17, 2014

@lgalfaso I updated the test to not break in IE. I guess attributes named 'hasOwnProperty' is not supported which makes sense. I still don't understand how non-IE passed though :|

@jbedard
Copy link
Contributor Author

jbedard commented Jan 26, 2015

Is this ok to merge without the breaking test?

@lgalfaso
Copy link
Contributor

LGTM

@jbedard jbedard force-pushed the compile-attr-obs-createmap branch from c6caf7d to b1246bf Compare February 14, 2015 05:40
jbedard referenced this pull request May 5, 2015
…lized from attr interpolation

This reverts commit 8e28bb4.
@caitp
Copy link
Contributor

caitp commented May 5, 2015

thanks @jbedard --- @lgalfaso any particular reason why this was not re-merged? I'll take a look tomorrow

@caitp caitp assigned caitp and unassigned lgalfaso May 5, 2015
@caitp caitp modified the milestones: 1.4.x - jaracimrman-existence, 1.3.x - superluminal-nudge May 5, 2015
petebacondarwin pushed a commit that referenced this pull request Sep 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants