feat(core): Mark components for check if they read a signal

[atscott]

This commit updates the LView in Angular to be a Consumer of signals. If a signal is read when executing a template, it marks the view dirty. In addition, if a signal is read when executing host bindings, it also marks views dirty.

One interesting thing about signal reads in host bindings is that they perform a bit better than what we can do with today's APIs. In order to re-execute host bindings for an OnPush component that might have changed, you would probably inject ChangeDetectorRef and call markForCheck. This will mark the current component and parents dirty. However, host bindings are executed as part of refreshing the parent so there is really no need to re-execute the current component if the only thing that changed is the host bindings. When a signal is read in host bindings, it marks the parent dirty and not the component that defined the host binding.

Additionally, this commit avoids allocating a full consumer for each LView by re-using a consumer until template execution results in a signal read. At this point, we assign that consumer to the LView and create a new consumer to "tentatively" use for the future LView template executions.

Co-authored-by: Dylan Hunn dylhunn@gmail.com

[Harpush]

If I am correct this uses the existing mechanism for dirty check like async pipe does. Are there any thoughts to change it to local change detection? So when a signal is "dirty" only the relevant template "pieces" are updated? For example if I have two divs in a component - one using a signal and the second calling a function from template, I guess the best case is only updating the signal bound div without the other template function executing when the signal is updated. This can propagate to sub components if the signal is bound to an input. That way instead of marking the component and all ancestors and dirty checking from the root - the change will start from the component using the signal and might end there or go down the tree due to inputs.

[atscott]

@Harpush Yes! @dylhunn and I prototyped this for signal-based components already and it's working pretty well. That said, we aren't going to do this for the existing change detection strategies (OnPush or Default) because we don't want to change the semantics of what it means to refresh those components. That is, change detection is owned by the component and the signal reads will conform to the dirty marking and change detection already in place for those strategies. When we release full signal-based component authoring in a future version, it will likely have per-view dirty marking.

Updated based on comments

[timonkrebs]

I am sorry! But i really feel this is relevant and since the discussion space is closed I feel like this is the best place to say it. And it really seems to be directly concerning the functionality of this PR.

Angularjs did automagically resolve promises. With angular 2+ the async pipe made it a lot more clear what is happening. And now we have an improved version of the async pipe: the ngrxPush pipe. We can even implement a custom pipe that handles the asynchronisity the way we need/want it to.

With this PR as I understand it it feels like we make the same automagically resolving mistake with signals that angularjs did with promises.

Please consider the power of handling signals with pipes! Use the full power that angular brings to the table! I think it will not only be more powerful but also feel more familiar. It would show, that it is not just a improved copy of solid js signals but that it actually fits perfectly into angular.

[atscott]

@timonkrebs These changes have nothing to do with promises. When a signal that was read in a template is updated, the component is marked for check. There is not anything here that integrates with ZoneJS and that is specifically a non-goal of the OnPush and CheckAlways integration. Please refer to the earlier explanation here #49153 (comment)

[timonkrebs]

@atscott I know that this has nothing to do with promises and ZoneJS. It is hard to describe my concern with it since I am not fully aware of how the final solution you are working towards should/will look like.

But marking all the components for check does point to a direction that signals would automagically trigger the rendering without having control over it. (from the perspective of an angular dev)

With the pipe approach that we have now for Observable<T>|Subscribable<T>|Promise<T> that can be consumed with the async pipe, we have the ability to write a custom pipe that handles these in any way we want. We are not forced to consume them in the way that the async pipe would.

With signals it will also be beneficial to have the same amount of control. For example in some cases it will be useful to control the scheduling of the rendering. We might not want to mark the component for check more than once per window.requestAnimationFrame cycle. This could be the case if we have computed values that are really expensive to calculate (but there are also other cases).

When a component automatically gets marked for check then the signals that are used inside this component will automatically be evaluated with the next tick. But this might not be what the developer needs/wants.

I think that marking a component for check should not be a concern of the renderer. It should be a lot closer to the reactivity.

[atscott]

When a component automatically gets marked for check then the signals that are used inside this component will automatically be evaluated with the next tick. But this might not be what the developer needs/wants.

This isn't entirely true. It is true that we will be marking the component for check automatically when a signal that the template reads is updated. This behavior is absolutely decided. The component is truly dirty and it is wrong to not mark it as dirty. markForCheck is the wrong way to control when change detection runs.

It is not true that the component will necessarily be checked during the next tick. This depends on whether you are using the default NgZone and if this view is still attached (ChangeDetectorRef.detach would mean the view would still not be refreshed along with an ApplicationRef.tick).

We do acknowledge the desire to have more control over the change detection scheduling and will be considering this more concretely in the designs for change detection without NgZone.

[timonkrebs]

Ok I think I get it now.

It is true that we will be marking the component for check automatically when a signal that the template reads is updated. This behavior is absolutely decided.

If the fine grained update mechanism is dependent on this behaviour then there is probably not much room to play with concerning the approach I mentioned. Probably the fine grained update solution you already worked out does work similar to what preact does with automatically converting the reactive parts in the template to micro components.

If this is the case I understand that the problem I would like to have improved on will likely not have a chance getting addressed in the way I thought about it.

For completeness sake I still would like to explain why I think it could be better to not mark the component as checked by the renderer.

The component is truly dirty and it is wrong to not mark it as dirty. markForCheck is the wrong way to control when change detection runs.

I am not entirely sure that I am on the same page with you about this. Currently if a component is not marked for check it does not mean that it is not dirty. That is why we can experience the NG0100: Expression has changed after it was checked...

So currently markForCheck is only a signal (pun intended) that the component probably is dirty (and something should be done about it) but if no one did markForCheck that does not mean that it can not be dirty anyway... (and markForCheck is not even a guarante that the component actually is dirty)

I think the mental model that you describe is really good. But there are and probably for a long time will be a lot of apps that do use NgZone. If you would like to migrate these apps, or like to get parts of apps to the full benefits of signals this will be an Issue.

As you mentioned there could be workarounds like ChangeDetectorRef.detach but this seems like a suboptimal solution... Maybe there could be a way to solve this with pipes nonetheless. But then it probably will not be related to this PR...

I actually think it is a real improvement when you can rely on components only being dirty if they are flagged as such. Sorry for the interference I did not anticipate how you would implement the fine grained part of signals.

The improvements you plan to introduce are even more fundamental than I thougt. In the long run I think this is very nice. But I think it does make the migration/partial use of signals a bit harder/less convenient... I get that the benefits in the long run do outweight the inconveniences when migrating (partially) to the new paradigm.

PS: I still am not completely convinced that marking components for check should be a concern of the renderer. But you say it is not debateable. So we do not have to go into that...

[atscott]

This PR was merged into the repository by commit 9b65b84.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}