-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(autocomplete): handle optionSelections
being accessed early
#8802
fix(autocomplete): handle optionSelections
being accessed early
#8802
Conversation
@@ -201,7 +201,15 @@ export class MatAutocompleteTrigger implements ControlValueAccessor, OnDestroy { | |||
|
|||
/** Stream of autocomplete option selections. */ | |||
get optionSelections(): Observable<MatOptionSelectionChange> { | |||
return merge(...this.autocomplete.options.map(option => option.onSelectionChange)); | |||
if (this.autocomplete && this.autocomplete.options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to make it so that optionSelections
is always the same observable, it just emits when ready instead of changing the reference to a different observable when it becomes ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would add some extra bloat since we'd have to maintain another property, as well drop all subscriptions and re-subscribe as the amount of options changes. Since this is mostly a convenience API, I'm not sure whether it's worth the trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably wanted to do:
optionSelections = defer(() => {
if (this.autoComplete && this.autoComplete.options) {
return merge(...this.autocomplete.options.map(option => option.onSelectionChange));
}
return this._zone.onStable
.asObservable()
.pipe(take(1), switchMap(() => this.optionSelections));
});
Remember, Observables are basically functions, you wouldn't create a getter that returned a new function reference every time. defer
is just a nice trick for making sure whatever the observable needs to be created exists when you subscribe to the observable.
Along the same lines angular#8802, `mat-select` will throw if the `optionSelectionChanges` is accessed before the options are initialized. These changes add a fallback that will replace the observable once the options become available.
e45ecb9
to
960d0d6
Compare
Along the same lines angular#8802, `mat-select` will throw if the `optionSelectionChanges` is accessed before the options are initialized. These changes add a fallback that will replace the observable once the options become available.
960d0d6
to
e9956c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Contains lint errors - can you fix these |
) Along the same lines #8802, `mat-select` will throw if the `optionSelectionChanges` is accessed before the options are initialized. These changes add a fallback that will replace the observable once the options become available.
e9956c2
to
cd11a49
Compare
Sorted out the lint warning. |
…gular#8830) Along the same lines angular#8802, `mat-select` will throw if the `optionSelectionChanges` is accessed before the options are initialized. These changes add a fallback that will replace the observable once the options become available.
@crisbeto looks like this has a unit test failure now |
Currently the `MatAutocompleteTrigger` will throw if `optionSelections` is accessed before `ngAfterViewInit`. These changes add a fallback stream that will be replaced once everything is initialized. Fixes angular#4616.
cd11a49
to
42a9593
Compare
Rebased and fixed the test failure. |
…gular#8802) Currently the `MatAutocompleteTrigger` will throw if `optionSelections` is accessed before `ngAfterViewInit`. These changes add a fallback stream that will be replaced once everything is initialized. Fixes angular#4616.
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 the
MatAutocompleteTrigger
will throw ifoptionSelections
is accessed beforengAfterViewInit
. These changes add a fallback stream that will be replaced once everything is initialized.Fixes #4616.