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

Error NG0601 is reported incorrectly (triggered by NG0600) when calling toSignal inside a computed or effect #51027

Closed
simeyla opened this issue Jul 13, 2023 · 6 comments
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

@simeyla
Copy link

simeyla commented Jul 13, 2023

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

Before I begin I want to stress this issue is regarding confusing error reporting and not the signals implementation or design itself. The situation described below is most likely to be encountered when migrating from Observables to Signals, and can lead to confusion.


Sample Code

Here is a simple enough computed.

It uses toSignal inside the computed, but let's assume the user is new to Signals and doesn't know they shouldn't do that..

// guaranteed synchronous observables
const store = {
  first$: of('Simon'),
  last$: of('Weaver')
};

// simple computed (inside injection context)
const fullName = computed(() => 
{
    const first = toSignal(store.first$, { requireSync: true });
    const last = toSignal(store.last$, { requireSync: true });

    return first() + ' ' + last();
});

console.log('full name ', fullName());

Output

The above code reports two errors, the first of which is incorrect and misleading.

Error: NG0601: toSignal() called with requireSync but Observable did not emit synchronously.
Error: NG0600: Writing to signals is not allowed in a computed or an effect by default. Use allowSignalWrites in the CreateEffectOptions to enable this inside effects.

It turns out the first error is a side effect of the second error (despite being printed out first). If you only notice the first error (and personally I like to put breakpoints inside the vendor code where they are raised!) then you will be misled.

Explanation

Angular error NG0601 is triggered when an Observable does not emit synchronously when calling toSignal.

  NG0601 `toSignal()` called with `requireSync` but `Observable` did not emit synchronously.

This works internally by subscribing to the observable and checking immediately to see if the corresponding signal has been assigned a value.

const sub = source.subscribe({
next: value => state.set({kind: StateKind.Value, value}),
error: error => state.set({kind: StateKind.Error, error}),
// Completion of the Observable is meaningless to the signal. Signals don't have a concept of
// "complete".
});

This sets a signal (which begins as StateKind.NoValue) to the synchronous value of the observable, which can then be checked if requireSync == true.

However since ALL writes to signals in computed and effect are forbidden this signal cannot be set, therefore it remains in StateKind.NoValue state, thus triggering the erroneous message:

  NG0601 `toSignal()` called with `requireSync` but `Observable` did not emit synchronously.

Possible Solution

This could easily be solved by introducing a boolean to track the synchronous emission instead of relying on the state of the internal toSignal signal:

  const syncValue = false;   // NEW

  const sub = source.subscribe({
    next: value => { syncValue = true; state.set({kind: StateKind.Value, value}); },
    error: error => { syncValue = true; state.set({kind: StateKind.Error, error}); },
    // Completion of the Observable is meaningless to the signal. Signals don't have a concept of
    // "complete".
  });

  if (ngDevMode && options?.requireSync && !syncValue) {
    throw new RuntimeError(
        RuntimeErrorCode.REQUIRE_SYNC_WITHOUT_SYNC_EMIT,
        '`toSignal()` called with `requireSync` but `Observable` did not emit synchronously.');
  }
@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime cross-cutting: signals labels Jul 13, 2023
@ngbot ngbot bot added this to the needsTriage milestone Jul 13, 2023
@pkozlowski-opensource
Copy link
Member

It is really similar to #51055 - in the sense that it creates a new reactive node in a computed. As with #51082 - we could detect it earlier, see a WIP change: pkozlowski-opensource@bce5132

I think that we should take a step back and think about which reactive nodes creation is allowed in which reactive context.

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Jul 24, 2023

For now we've got, roughly, those types of reactive nodes and we should try to answer one question: does it make sense to create an instance of a given reactive node while another reactive node is an active consumer. Those are the options - for some of the combinations the answer is pretty straightforward - for others we could debate:

consumer / producer signal computed effect template effect
signal na na na na
computed na
effect na
template effect na

✅ - allow
❌ - disallow
na - not applicable (should never happen)

@pkozlowski-opensource pkozlowski-opensource added the core: reactivity Work related to fine-grained reactivity in the core framework label Jul 24, 2023
@pkozlowski-opensource
Copy link
Member

Minimal reproduction scenario: https://stackblitz.com/edit/stackblitz-starters-w46vxb

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Sep 20, 2023
This PR moves the Observable subscription of toSignal outside of the
reactive context. As the result the toSignal calls are allowed in the
computed, effect and all other reactive consumers.

This is based on the reasoning that we already allow signals creation
in a reactive context. Plus a similar change was done to the async pipe
in the angular#50522

Fixes angular#51027
@pkozlowski-opensource
Copy link
Member

@simeyla #51831 - WDYT?

@simeyla
Copy link
Author

simeyla commented Sep 21, 2023

Looks great. I do hope people in general won't end up using toSignal() inside a computed though, unless caching it at the same time.

But given that they will, makes me wonder if there should be a companion utility function to get the observable value one time.

If people end up doing toSignal(obs$, { requireSync: true }) () it's pretty ugly.

A utility function, something like syncValue(obs$) wouldn't need to create an intermediate signal and be clearer in intent.

In my 'journey' with nested computed I had to write my own safety checker (expecting #51082 to be imminent) and it actually saved me quite a few times. I found places where I was creating new nodes that wasn't immediately obvious.

I'm guessing you decided not to put this behind a flag (?) but I feel like if there was one it could catch quite a few bugs.

Also, this separate issue can now be marked as fixed: #50106

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Sep 25, 2023
This PR moves the Observable subscription of toSignal outside of the
reactive context. As the result the toSignal calls are allowed in the
computed, effect and all other reactive consumers.

This is based on the reasoning that we already allow signals creation
in a reactive context. Plus a similar change was done to the async pipe
in the angular#50522

Fixes angular#51027
dylhunn pushed a commit that referenced this issue Sep 27, 2023
This PR moves the Observable subscription of toSignal outside of the
reactive context. As the result the toSignal calls are allowed in the
computed, effect and all other reactive consumers.

This is based on the reasoning that we already allow signals creation
in a reactive context. Plus a similar change was done to the async pipe
in the #50522

Fixes #51027

PR Close #51892
@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 Oct 23, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
This PR moves the Observable subscription of toSignal outside of the
reactive context. As the result the toSignal calls are allowed in the
computed, effect and all other reactive consumers.

This is based on the reasoning that we already allow signals creation
in a reactive context. Plus a similar change was done to the async pipe
in the angular#50522

Fixes angular#51027

PR Close angular#51831
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

Successfully merging a pull request may close this issue.

2 participants