fix($compile): exception when using "watch" as isolated scope binding variable in Firefox #11628
Conversation
randing89
changed the title
randing89
changed the title
|
#11627 looks like a real issue and the diagnose also looks accurate, but this fix does not look right:
|
|
@lgalfaso Thanks for your comment. I have revised my pull request and added few test cases. |
|
@lgalfaso |
| switch (mode) { | ||
|
|
||
| case '@': | ||
| if (!attrs[attrName]) { | ||
| if (optional) { | ||
| break; |
There was a problem hiding this comment.
so if the value is optional and it does not have an initial value, then we are not observing the attribute? This implies that
<div attr-abc="{{abc}}" some-directive-that-binds-to-the-attribute-abc></div>
will stop working. This is a breaking change
|
On top, a test that breaks in Firefox using the property |
|
One more thing, please rebase and squash everything into one commit Thanks! |
|
I think we should just createMap the Attributes class' prototype so that we don't have to deal with non-standard Object.prototype properties. Unfortunately, doing that is a breaking change |
This would make the code simpler, but given that it is a breaking change, I would like other opinions |
|
I have revised the patch. I set the |
| @@ -2545,9 +2545,19 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| lastValue, | |||
| parentGet, parentSet, compare; | |||
|
|
|||
| if ((attrName in attrs) && !attrs.hasOwnProperty(attrName)) { | |||
There was a problem hiding this comment.
Please change this to if (hasOwnProperty(attrs, attrName)) {
There was a problem hiding this comment.
You mean hasOwnProperty.call(attrs, attrName), right ?
|
Otherwise, LGTM |
|
Updated. Thanks for the review |
| @@ -2545,9 +2545,19 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| lastValue, | |||
| parentGet, parentSet, compare; | |||
|
|
|||
| if ((attrName in attrs) && !hasOwnProperty.call(attrs, attrName)) { | |||
There was a problem hiding this comment.
is (attrName in attrs) && needed?
There was a problem hiding this comment.
Yes. In the case attrs === {} and attrName === 'watch' we want to make sure code in the switch below accessing attrs['watch'] instead of attrs.prototype.watch. So this is use to find out if attrName is in the prototype chain but not actually defined in attrs object.
There was a problem hiding this comment.
I mean, is if (!hasOwnProperty.call(attrs, attrName)) { good?
There was a problem hiding this comment.
Em... Tried to run the test and seems all good. I've updated the commit. Thanks.
… variable in Firefox Fix on all binding modes: '=', '@' and '&' as well as optional cases Throw exception when user define 'hasOwnProperty' in binding. Closes #11627
|
LGTM, will merge right after we cut the next release. |
|
landed as a6339d3 |
Firefox throws "Error: can't convert undefined to object" exception when using "watch" as a isolated scope binding
It is because in Firefox Object.prototype.watch is defined and the related code is not using hasOwnProperty to check property availability.
Closes #11627