fix(select): avoid going into infinite loop under certain conditions #2955
Conversation
googlebot
added
the
cla: yes
src/lib/select/select.ts
Outdated
| this._tempValue = null; | ||
| }); | ||
| } | ||
|
|
||
| this._changeSubscription = this.options.changes.subscribe(() => { |
There was a problem hiding this comment.
Given that we are doing the same thing on option changes (i.e. writing the value next tick), I think it might make more sense to just incorporate the logic into the existing stream with rx operatorstartWith(). this._control.value should have the correct value already, so saving a temp version shouldn't be necessary.
There was a problem hiding this comment.
Makes sense, I'll rework it. This was already being handled in a similar way on the multiple selection PR.
|
Regarding unit tests, this is a little hard to test, because the tests would either crash (if the infinite loop happens) or throw a runtime error (if we avoided the loop). That being said, we already have some tests that will fail if we don't handle the initial value properly. |
|
@crisbeto How about you use the pattern expect(() => {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
}).toThrowError(new RegExp(...));If it passes, we know it's good. If the tests do crash, obviously there's a problem. |
|
Makes sense, but it probably needs an extra comment about it as well. |
|
Added the test and used your approach with EDIT: Turns out it was a button from another test not being cleaned up. |
Currently `md-select` calls `writeValue` recursively until the `QueryList` with all of the options is initialized. This usually means 1-2 iterations max, however in certain conditions (e.g. a sibling component throws an error on init) this may not happen and the browser would get thrown into an infinite loop. This change switches to saving the attempted value assignments in a property and applying it after initialization. Fixes angular#2950.
crisbeto
force-pushed
the
2950/select-infinite-loop
branch
from
707a90a
to
bd6f080
Compare
src/lib/select/select.ts
Outdated
| @@ -242,7 +244,8 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr | |||
| ngAfterContentInit() { | |||
| this._initKeyManager(); | |||
| this._resetOptions(); | |||
There was a problem hiding this comment.
You should be able to remove this call now that it's happening again in line 249
There was a problem hiding this comment.
Done. Should I re-label?
kara
added
pr: lgtm
action: merge
|
@crisbeto Can you rebase? |
# Conflicts: # src/lib/select/select.spec.ts # src/lib/select/select.ts
crisbeto
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently
md-selectcallswriteValuerecursively until theQueryListwith all of the options is initialized. This usually means 1-2 iterations max, however in certain conditions (e.g. a sibling component throws an error on init) this may not happen and the browser would get thrown into an infinite loop.This change switches to saving the attempted value assignments in a property and applying it after initialization.
Fixes #2950.