fix($compile): ignore optional =-bound properties with empty value #12290
Conversation
…tribute is not present"" This reverts commit d193c3a.
|
@petebacondarwin / @jeffbcross PTAL --- I've tested this against the angular-material tests and it seems to be working okay, but I'm not sure what the issue was with the tab control (so it would be good to get another look just in case there's another sneaky bug) |
Previously, optional empty '='-bound expressions were ignored --- erroneously they stopped being ignored, and no tests were caused to fail for this reason. This change restores the original ignoring behaviour while still preventing other errors fixed by 8a1eb16 Closes angular#12144 Closes angular#12259
|
Thanks @caitp ! It looks like @ThomasBurleson is involved with this, too. |
|
is travis really being this slow today? wow |
| @@ -2592,11 +2587,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| break; | |||
|
|
|||
| case '=': | |||
| if (optional && !attrs[attrName]) { | |||
| return; | |||
| if (!hasOwnProperty.call(attrs, attrName)) { | |||
There was a problem hiding this comment.
Why not just use attrs.hasOwnProperty (instead of hasOwnProperty.call(attrs) here and above ?
(It is being used below anyway ;))
There was a problem hiding this comment.
attrs shouldn't share [[Prototype]] with Object, there are a lot of bugs which have been fixed because of this. the code below just came from a revert of a revert, so it's part of the pieces that haven't been fixed yet
There was a problem hiding this comment.
@caitp - in #12144 (comment) you mentioned some additional code required for Angular Material.
- Is that still true?
- If yes, should we modify the Angular Material directives with optional 2-way binding?
There was a problem hiding this comment.
The code in that snippet is contained in the second commit here, just organized slightly differently --- I decided to avoid the isString() check and just test if it's falsy (values in attrs should always be a string, unless it gets shadowed above, in which case it's undefined --- so also falsy) --- so the simpler version in this PR should be fine
There was a problem hiding this comment.
Oh, the example snippet in that issue also handles (ignores) values which are exclusively whitespace, but I think these are pretty rare
There was a problem hiding this comment.
@ThomasBurleson - see @caitp's reply to your question, that the code snippet that is mentioned in the previously linked comment is included in this PR. In other words you shouldn't need to make changes to Material for all your (non-disabled) tests to pass.
I think the only thing we can do now is see if this PR works for the latest on Material's master branch and if so then merge this PR.
There was a problem hiding this comment.
@petebacondarwin - Re: ... see if this PR works for the latest on Material's master branch. Is this something I should be doing now; or is someone else assigned this task ?
There was a problem hiding this comment.
@ThomasBurleson would be nice if you could test that!
There was a problem hiding this comment.
@Narretz - I will test tonight and respond tomorrow. Will that work ?
|
Without running the tests and stuff myself, this LGTM |
|
I'm a bit worried that it could break things in ways that aren't being tested in Material. @jeffbcross did you know of any demo apps in material which were affected? The ones I tried all looked fine (for the tabs component, autocomplete component, and chips components) |
|
I don't know any demo apps that were affected, but I have limited familiarity with the material project. Could you give @ThomasBurleson some guidance on where this change could potentially cause problems? |
|
@jeffbcross, @petebacondarwin - see comment https://github.com/angular/angular.js/pull/12290/files#r34271903 |
|
is this being actively investigated ? we are blocked by this for our next release and would have to change a number of sources to workaround. we would like to avoid that if this is being expected to be merged soon |
|
This PR still causes observers to be called with |
|
it only does in certain condition, when the listeners are not optional. We don't have a great other way to do this |
|
I say, let's merge it in time for 1.4.4. We can always look into this observers fired with undefined thing later. |
|
just merge this in .. this is causing a number of issues already and is making some directives quite cumbersome to write |
|
A project for tomorrow for me. OK |
|
Thanks Pete |
Previously, optional empty '='-bound expressions were ignored --- erroneously they stopped being ignored, and no tests were caused to fail for this reason. This change restores the original ignoring behaviour while still preventing other errors fixed by 8a1eb16 Closes angular#12144 Closes angular#12259 Closes angular#12290
Previously, optional empty '='-bound expressions were ignored --- erroneously they stopped being ignored, and no tests were caused to fail for this reason. This change restores the original ignoring behaviour while still preventing other errors fixed by 8a1eb16 Closes angular#12144 Closes angular#12259 Closes angular#12290
Previously, optional empty '='-bound expressions were ignored ---
erroneously they stopped being ignored, and no tests were caused to fail
for this reason. This change restores the original ignoring behaviour while
still preventing other errors fixed by 8a1eb16
Closes #12144
Closes #12259