-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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 ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(elements): switch toComponentRef.setInput
& remove custom scheduler
#56728
base: main
Are you sure you want to change the base?
Conversation
@alxhubMay I know when do we get this in? Is there something more which needs to be done here? |
9859e12
to
f70a693
Compare
4188f72
to
1855574
Compare
constapplicationRef=this.injector.get<ApplicationRef>(ApplicationRef); | ||
applicationRef.attachView(this.componentRef.hostView); | ||
this.appRef.attachView(this.componentRef.hostView); | ||
this.componentRef.hostView.detectChanges(); |
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.
I think you should flip the order here -detectChanges
then attach. It matches what was there before, but more importantly, it clears the dirty bits from the host view by refreshing it before attaching to the application, so it prevents a notification to the scheduler.
…duler The custom element implementation previously used a custom code path for setting inputs, which contained bespoke code for writing input properties, detecting whether inputs actually change, marking the component dirty, scheduling and running CD, invoking `ngOnChanges`, etc. This custom logic had several downsides: * Its behavior different from how Angular components behave in a normal template. For example, inputs setters were invoked in `NgZone.run`, which (when called from outside the zone) would trigger synchronous CD in the component, _without_ calling `ngOnChanges`. Only when the custom rAF- scheduled `detectChanges()` call triggered would `ngOnChanges` be called. * CD always ran multiple times, because of the above. `NgZone.run` would trigger CD, and then separately the scheduler would trigger CD. * Signal inputs were not supported, since inputs were set via direct property writes. This change refactors the custom element implementation with two changes: 1. `ComponentRef.setInput` is used instead of a custom code path for writing inputs. This allows us to drop all the custom logic related to managing `ngOnChanges`, since `setInput` does that under the hood. `ngOnChanges` behavior now matches how the component would behave when _not_ rendered as a custom element. 2. The custom rAF-based CD scheduler is removed in favor of the main Angular scheduler, which now handles custom elements as necessary. Running `NgZone.run` is sufficient to trigger CD when zones are used, and the hybrid zoneless scheduler now ensures CD is scheduled when `setInput` is called even with no ZoneJS enabled. As a result, the dedicated elements scheduler is now only used when Angular's built-in scheduler is disabled. As a concession to backwards compatibility, the element's view is also marked for refresh when an input changes. Doing this allows CD to revisit the element even if it becomes dirty during CD, mimicking how it would be detected by the former elements scheduler unconditionally refreshing the view a second time. As a part of this change, the elements tests have been significantly refactored. Previously all of Angular was faked/spied, which had a number of downsides. For example, there were tests which asserted that change detection only happens once when setting multiple inputs. This wasn't actually the case (because of `NgZone.run` - see logic above) but the test didn't catch the issue because it was only spying on `detectChanges()` which isn't called from `ApplicationRef.tick()`. Even the components were fake. Now, the tests use real Angular components and factories. They've also been updated to not use `fakeAsync`. A number of tests have been disabled, which were previously asserting behavior that wasn't matching what was actually happening (as above). Other tests were disabled due to real differences with `ngOnChanges` behavior, where the current behavior could be seen as a bug. Fixesangular#53981
Hi@chintankavathia,thanks for your interest! Testing of this change in Google applications revealed a subtle difference in the scheduling behavior - previously, Elements was hiding/mitigating real ChangeAfterChecked errors which were now correctly caught with the new behavior. Unfortunately we consider this a breaking change, so it took some time to figure out a reasonable fix (thanks,@atscott!). The change now has the fix logic (which you can see in the new test). There's no guarantee about when it will land as internal testing may reveal even more problems, but I'm hopeful it will make it into the next minor release. |
this.componentRef!.setInput(this.inputMap.get(property)??property,value); | ||
|
||
// `setInput` won't mark the view dirty if the input didn't change from its previous value. | ||
if((this.componentRef!.hostViewasViewRef<unknown>).dirty){ |
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.
Just a note that this doesn't necessarily mean thesetInput
caused the host view to become dirty since it might have already been dirty before setting the input. Not sure if it's worth "fi xing" this.
The custom element implementation previously used a custom code path for
setting inputs, which contained bespoke code for writing input properties,
detecting whether inputs actually change, marking the component dirty,
scheduling and running CD, invoking
ngOnChanges
,etc. This custom logichad several downsides:
Its behavior different from how Angular components behave in a normal
template.
For example, inputs setters were invoked in
NgZone.run
,which (whencalled from outside the zone) would trigger synchronous CD in the
component,withoutcalling
ngOnChanges
.Only when the custom rAF-scheduled
detectChanges()
call triggered wouldngOnChanges
be called.CD always ran multiple times, because of the above.
NgZone.run
wouldtrigger CD, and then separately the scheduler would trigger CD.
Signal inputs were not supported, since inputs were set via direct
property writes.
This change refactors the custom element implementation with two changes:
ComponentRef.setInput
is used instead of a custom code path forwriting inputs.
This allows us to drop all the custom logic related to managing
ngOnChanges
,sincesetInput
does that under the hood.ngOnChanges
behavior now matches how the component would behave whennotrendered
as a custom element.
is disabled.
Running
NgZone.run
is sufficient to trigger CD when zones are used, andthe hybrid zoneless scheduler now ensures CD is scheduled when
setInput
iscalled even with no ZoneJS enabled. As a result, the dedicated elements
scheduler is now only used when Angular's built-in scheduler is disabled.
As a part of this change, the elements tests have been significantly
refactored. Previously all of Angular was faked/spied, which had a number
of downsides. For example, there were tests which asserted that change
detection only happens once when setting multiple inputs. This wasn't
actually the case (because of
NgZone.run
- see logic above) but the testdidn't catch the issue because it was only spying on
detectChanges()
whichisn't called from
ApplicationRef.tick()
.Even the components were fake.Now, the tests use real Angular components and factories. They've also been
updated to not use
fakeAsync
.A number of tests have been disabled, which were previously asserting
behavior that wasn't matching what was actually happening (as above). Other
tests were disabled due to real differences with
ngOnChanges
behavior,where the current behavior could be seen as a bug.
Fixes#53981