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

effect and computed throws circular dependency when ErrorHandler creates effect computed during construction (or one of its dependencies does) #52680

Closed
blueiceprj opened this issue Nov 8, 2023 · 12 comments
Assignees
Labels
area: core Issues related to the framework runtime bug core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: has PR
Milestone

Comments

@blueiceprj
Copy link

blueiceprj commented Nov 8, 2023

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

core

Is this a regression?

Yes

Description

After upgrade to 17 signal objects not working properly

Injectable({ providedIn: 'root' })
export class PrincipalService {
    _loginCompleted = signal<boolean>(false);
    _token = signal<string>(null);
    user = signal<SessionState>(createInitialSessionState());
    user$ = toObservable(this.user);
apiCount = signal<number>(0);
constructor() {
        effect(() => {
            setTimeout(() => {
                this.visible.set(this.apiCount() != 0);
            }, 0);
        }, {
            allowSignalWrites: true
        });
    }

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

Error: NG0200: Circular dependency in DI detected for ErrorHandler. Find more at https://angular.io/errors/NG0200
    at throwCyclicDependencyError (core.mjs:292:11)
    at R3Injector.hydrate (core.mjs:6158:13)
    at R3Injector.get (core.mjs:6033:33)
    at effect (core.mjs:14259:35)
    at toObservable (rxjs-interop.mjs:48:27)
    at new PrincipalService (principal.service.ts:22:25)
    at Object.PrincipalService_Factory [as factory] (ɵfac.js:1:1)
    at core.mjs:6164:43
    at runInInjectorProfilerContext (core.mjs:867:9)
    at R3Injector.hydrate (core.mjs:6163:17)



Error: NG0200: Circular dependency in DI detected for ErrorHandler. Find more at https://angular.io/errors/NG0200
    at throwCyclicDependencyError (core.mjs:292:11)
    at R3Injector.hydrate (core.mjs:6158:13)
    at R3Injector.get (core.mjs:6033:33)
    at effect (core.mjs:14259:35)
    at new FuseProgressBarService (progress-bar.service.ts:24:15)
    at Object.FuseProgressBarService_Factory [as factory] (ɵfac.js:1:1)
    at core.mjs:6164:43
    at runInInjectorProfilerContext (core.mjs:867:9)
    at R3Injector.hydrate (core.mjs:6163:17)
    at R3Injector.get (core.mjs:6033:33)

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 17.0.0
Node: 18.18.2
Package Manager: npm 9.8.1
OS: darwin x64

Angular: 17.0.0
... animations, cli, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.1700.0
@angular-devkit/build-angular      17.0.0
@angular-devkit/build-ng-packagr   0.1002.0
@angular-devkit/core               17.0.0
@angular-devkit/schematics         17.0.0
@angular/cdk                       16.2.12
@angular/flex-layout               15.0.0-beta.42
@angular/material                  16.2.12
@angular/material-moment-adapter   16.2.12
@angular/youtube-player            16.2.12
@schematics/angular                17.0.0
rxjs                               7.8.1
typescript                         5.2.2
zone.js                            0.14.2

Anything else?

No response

@atscott
Copy link
Contributor

atscott commented Nov 8, 2023

Can you provide a reproduction? The effect implementation attempts to inject ErrorHandler so I'm assuming you are using an effect inside a class that your custom ErrorHandler also injects.

Edit: Here's a reproduction. Can you confirm that this is similar to your application?

@atscott atscott added area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework labels Nov 8, 2023
@ngbot ngbot bot modified the milestone: needsTriage Nov 8, 2023
@blueiceprj
Copy link
Author

Sorry! Its a huge project. Thats just part of code I shared. They are just root injected services. But these services call by custom error handler.

@alxhub alxhub added the needs reproduction This issue needs a reproduction in order for the team to investigate further label Nov 8, 2023
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Nov 8, 2023
@atscott atscott removed the needs reproduction This issue needs a reproduction in order for the team to investigate further label Nov 8, 2023
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Nov 8, 2023
@atscott atscott changed the title signal effect and toObservable objects throws error effect throws circular dependency when ErrorHandler creates effect during construction (or one of its dependencies does) Nov 8, 2023
@blueiceprj
Copy link
Author

blueiceprj commented Nov 8, 2023

Ok. I figure out that. Same error.

https://stackblitz.com/edit/stackblitz-starters-ppn9ig?file=src%2Fmain.ts

@blueiceprj blueiceprj changed the title effect throws circular dependency when ErrorHandler creates effect during construction (or one of its dependencies does) effect and computed throws circular dependency when ErrorHandler creates effect computed during construction (or one of its dependencies does) Nov 8, 2023
@blueiceprj
Copy link
Author

Still not working with last version. Is there any news?

@pkozlowski-opensource
Copy link
Member

We can mitigate this situation by getting the ErrorHandler on-demand in the effect. Marking it as a bug than we can fix.

@pkozlowski-opensource pkozlowski-opensource added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent bug labels Dec 13, 2023
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Dec 13, 2023
@crisbeto crisbeto self-assigned this Dec 28, 2023
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 28, 2023
`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 28, 2023
`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.
atscott pushed a commit that referenced this issue Jan 3, 2024
`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes #52680.

PR Close #53713
@atscott atscott closed this as completed in 2b9a850 Jan 3, 2024
@blueiceprj
Copy link
Author

Fixed. Thanks guys

@blueiceprj
Copy link
Author

blueiceprj commented Jan 11, 2024

However, if you use ErrorHandler, there is a problem with this too. A simple data type, string, boolean etc. If I use them, it works, but when I change the data type with a complex interface like in this example, effect doesn't work. It works properly if you remove ErrorHandler.

Please check this example https://stackblitz.com/edit/stackblitz-starters-2fvmzs?file=src%2Fmain.ts

@atscott

@atscott
Copy link
Contributor

atscott commented Jan 11, 2024

@blueiceprj I don't understand what your demo is trying to show or what you're saying isn't working. Can you explain this more?

@blueiceprj
Copy link
Author

Same example. But I changed signal object from primitive type to object type like string to LoginState

export interface UserState {
  loggedIn: boolean;
  username: string;
}

And try to change properties of this class (loggedIn, username) like that.

effect(
      () => {
        console.log('Changed', this.loginState());
      },
      {
        allowSignalWrites: true,
      }
    );
.............................
.......................
login() {
    this.loginState.update((v) => {
      /* BUT IT DOESN'T WORK */
      /*
      v.loggedIn = true;
      v.username = 'anyone';
      return v;
    */
      /* BUT IT WORKS */
      const newLogin = { ...v };
      newLogin.loggedIn = true;
      newLogin.username = 'anyone';
      return newLogin;
    });
  }

First Version

Ekran Resmi 2024-01-11 23 10 32

Second version

Ekran Resmi 2024-01-11 23 09 30

Result : I always need to copy of signal value to trigger effect function.

@atscott
Copy link
Contributor

atscott commented Jan 11, 2024

Result : I always need to copy of signal value to trigger effect function.

Ah, I see - I was expecting this to be related to the ErrorHandler again. This is about signal updates and mutability. The default equality function for signals is comparing by reference. If you want a mutable signal, you have to change its equality function. I've updated the example here

@blueiceprj
Copy link
Author

Ok. I see. Thanks.

ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
)

`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.

PR Close angular#53713
rlmestre pushed a commit to rlmestre/angular that referenced this issue Jan 26, 2024
)

`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.

PR Close angular#53713
danieljancar pushed a commit to danieljancar/angular that referenced this issue Jan 26, 2024
)

`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.

PR Close angular#53713
amilamen pushed a commit to amilamen/angular that referenced this issue Jan 26, 2024
)

`effect` was expecting an `ErrorHandler` in its constructor which can lead to a circular DI error if an effect is used inside a custom `ErrorHandler`. These changes inject the `ErrorHandler` only when reporting errors.

Fixes angular#52680.

PR Close angular#53713
@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 Feb 11, 2024
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 bug core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: has PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants