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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "equal" field to ToSignalOptions #52176

Closed
e-oz opened this issue Oct 12, 2023 · 9 comments
Closed

Add "equal" field to ToSignalOptions #52176

e-oz opened this issue Oct 12, 2023 · 9 comments
Assignees
Labels
area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Milestone

Comments

@e-oz
Copy link

e-oz commented Oct 12, 2023

Which @angular/* package(s) are relevant/related to the feature request?

core

Description

Consider adding "equal" field to the ToSignalOptions interface, to let users override the equality check function of the created signal.

Proposed solution

ToSignalOptions with equal

Alternatives considered

Can't find any.

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime cross-cutting: signals labels Oct 12, 2023
@ngbot ngbot bot added this to the needsTriage milestone Oct 12, 2023
@pkozlowski-opensource pkozlowski-opensource added the core: reactivity Work related to fine-grained reactivity in the core framework label Oct 12, 2023
@pkozlowski-opensource
Copy link
Member

Consider adding "equal" field to the ToSignalOptions interface, to let users override the equality check function of the created signal.

We could, although I wonder if it is not better handled at the Observable chain level.

@e-oz
Copy link
Author

e-oz commented Oct 14, 2023

handled at the Observable chain level

At that level, you can control if an observable should emit, but you'll need to create a copy of the produced value if you want to "force" a signal to emit a notification.
In comparison to the requested feature, this option has a higher performance cost, and the code should be either custom for every case, or should have additional checks, like this:

      map((obj) => {
        if (obj == null) {
          return obj;
        }
        if (typeof obj === 'object') {
          if (Array.isArray(obj)) {
            return obj.slice();
          }
          if (obj instanceof Map) {
            return new Map(obj);
          }
          if (obj instanceof Set) {
            return new Set(obj);
          }
          return { ...obj };
        }
        return obj;
      }),

(this code is just an illustration, it's not tested or optimized)

Also, there are some objects that can not be cloned (even using structuredClone()), and they need their prototype chains (so they should not be shallow-copied). For example, DOM nodes.

@alxhub
Copy link
Member

alxhub commented Oct 16, 2023

I don't think custom equality solves this problem. In particular, custom equality can decide that two referentially unequal values are still "equal" and block the notification, but it cannot decide that a single value has changed if its object identity is the same. That is, equal(x, x) should always be true.

This makes me wonder if we should circumvent custom equality entirely when the value is referentially equivalent...

@e-oz
Copy link
Author

e-oz commented Oct 17, 2023

but it cannot decide that a single value has changed if its object identity is the same.

Of course it can. Map remains the same object when you modify one of its keys. DOM node remains the same when you modify children. And these are examples of structures that are pretty valid in the role of mutable structures. Immutability has performance costs, and there are cases when you want to say to signal “Yes, it is the same object, but it was updated - I know it for sure”.

if we should circumvent custom equality entirely when the value is referentially equivalent

That would make signals unusable for mutable data structures.

@e-oz
Copy link
Author

e-oz commented Oct 17, 2023

There is another case (that is most important for me): you want the signal to send a notification every time observable emits a new value, even if this value (accidentally) is the same object.

@pkozlowski-opensource pkozlowski-opensource self-assigned this Oct 18, 2023
@alxhub
Copy link
Member

alxhub commented Oct 18, 2023

That would make signals unusable for mutable data structures.

We removed .mutate for signals precisely for this reason. In particular, when signals and change detection interact, the mutability story falls apart.

Suppose you have a source() signal, and bind it to a sink input:

<sink-cmp [sink]="source()" />

If sink is received as an input signal, then this is effectively behaving during change detection as:

sink.set(source())`

The problem is that mutability gets lost here - change detection will only set sink if source() changes its referential identity, regardless of how often the source signal notifies of changes. Effectively, mutable change notifications are lost at this boundary.

@e-oz
Copy link
Author

e-oz commented Oct 18, 2023

But signals are not only for change detection. There might be layers and layers of computed signals before the result is consumed by the change detection logic.

I’ll quote my tweet:

This strategy would make sense if Angular Signals were a Change Detection Reactivity Primitive, not a General Reactivity Primitive.

At this moment, I even think that Angular Signals would evolve better outside of Angular repo - as a separate library, like RxJS, and it would make them framework-agnostic, and they would evolve as a General Reactivity Primitive.

If you convert signals into a Change Detection Reactivity Primitive, they will only be used as the “async” pipe - users will convert observables into signals only at the last step when they go to the template.

Maybe it's exactly what you want, I don't know.

@e-oz
Copy link
Author

e-oz commented Nov 23, 2023

This request has no sense after #52465 ;)

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Projects
None yet
Development

No branches or pull requests

3 participants