Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jun 26, 2024

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, invokingngOnChanges,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 inNgZone.run,which (when
    called from outside the zone) would trigger synchronous CD in the
    component,withoutcallingngOnChanges.Only when the custom rAF-
    scheduleddetectChanges()call triggered wouldngOnChangesbe called.

  • CD always ran multiple times, because of the above.NgZone.runwould
    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.setInputis used instead of a custom code path for
    writing inputs.

This allows us to drop all the custom logic related to managing
ngOnChanges,sincesetInputdoes that under the hood.ngOnChanges
behavior now matches how the component would behave whennotrendered
as a custom element.

  1. The custom rAF-based CD scheduler is only used when the hybrid scheduler
    is disabled.

RunningNgZone.runis sufficient to trigger CD when zones are used, and
the hybrid zoneless scheduler now ensures CD is scheduled whensetInputis
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 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 ofNgZone.run- see logic above) but the test
didn't catch the issue because it was only spying ondetectChanges()which
isn't called fromApplicationRef.tick().Even the components were fake.

Now, the tests use real Angular components and factories. They've also been
updated to not usefakeAsync.

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 withngOnChangesbehavior,
where the current behavior could be seen as a bug.

Fixes#53981

@angular-robot angular-robot bot added the area: elements Issues related to Angular Elements label Jun 27, 2024
@ngbot ngbot bot added this to theBacklogmilestone Jun 27, 2024
@chintankavathia
Copy link

@alxhubMay I know when do we get this in? Is there something more which needs to be done here?

@alxhub alxhub added the target: patch This PR is targeted for the next patch release label Jul 16, 2024
@alxhub alxhub marked this pull request as ready for review July 16, 2024 16:14
@alxhub alxhub force-pushed the signal-elements branch 3 times, most recently from 9859e12 to f70a693 Compare July 16, 2024 17:15
@alxhub alxhub added the requires: TGP This PR requires a passing TGP before merging is allowed label Jul 16, 2024
@pullapprove pullapprove bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label Jul 16, 2024
@alxhub alxhub force-pushed the signal-elements branch 2 times, most recently from 4188f72 to 1855574 Compare July 17, 2024 13:48
constapplicationRef=this.injector.get<ApplicationRef>(ApplicationRef);
applicationRef.attachView(this.componentRef.hostView);
this.appRef.attachView(this.componentRef.hostView);
this.componentRef.hostView.detectChanges();
Copy link
Contributor

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 -detectChangesthen 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.

@chintankavathia
Copy link

@alxhub,@atscottCan we expect this to be in the next patch?

…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
@alxhub
Copy link
Member Author

alxhub commented Jul 30, 2024

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){
Copy link
Contributor

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 thesetInputcaused 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: elements Issues related to Angular Elements target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supportinput(),output()andmodel()in Angular elements
3 participants