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

🐞[BUG]: 'Error: You have forgotten to import the NGXS module' after upgrading to 3.7.4 #1854

Open
willocho opened this issue Jun 9, 2022 · 31 comments

Comments

@willocho
Copy link

willocho commented Jun 9, 2022

Affected Package

The issue is caused by package @ngxs/3.7.4

Is this a regression?

Yes, the previous version in which this bug was not present was: 3.7.3

Description

After upgrading to 3.7.4, released earlier today on npm, our angular tests started failing.

🔬 Minimal Reproduction

The test would fail for a class that simply used the Select decorator, or depended on a class that used the Select decorator.

Minimally reproducible example:


import { TestBed } from "@angular/core/testing";

import { ScreenLayoutService } from "./screen-layout.service";
import { NgxsModule } from "@ngxs/store";
import { StateLocal } from "@shared/models/state/client/client-state.local";
import { StateSession } from "@shared/models/state/client/client-state.session";
import { StateRoot } from "@shared/models/state/state.main";
import { StateMirror } from "@shared/models/state/mirror/state.mirror";

describe("ScreenLayoutService", () => {
    beforeEach(() => TestBed.configureTestingModule({
        imports: [
            NgxsModule.forRoot([StateRoot, StateMirror, StateLocal, StateSession]),
        ],
        providers: [ ScreenLayoutService ]
    }));

    it("should be created", () => {
        const service: ScreenLayoutService = TestBed.get(ScreenLayoutService);
        expect(service).toBeTruthy();
    });
});

Where ScreenLayoutService is a an Angular service that uses @select

https://stackblitz.com/...

🔥 Exception or Error




Chrome Headless 100.0.4896.88 (Linux x86_64) ERROR
  An error was thrown in afterAll
  error properties: Object({ longStack: 'Error: You have forgotten to import the NGXS module!
      at throwSelectFactoryNotConnectedError (http://localhost:9876/_karma_webpack_/webpack:/node_modules/@ngxs/store/__ivy_ngcc__/fesm2015/ngxs-store.js:73:1)
      at createSelectObservableIvy (http://localhost:9876/_karma_webpack_/webpack:/node_modules/@ngxs/store/__ivy_ngcc__/fesm2015/ngxs-store.js:3844:1)
      at createSelectObservable (http://localhost:9876/_karma_webpack_/webpack:/node_modules/@ngxs/store/__ivy_ngcc__/fesm2015/ngxs-store.js:3800:1)
      at MergeMapSubscriber.project (http://localhost:9876/_karma_webpack_/webpack:/node_modules/@ngxs/store/__ivy_ngcc__/fesm2015/ngxs-store.js:3931:1)
      at MergeMapSubscriber._tryNext (http://localhost:9876/_karma_webpack_/webpack:/node_modules/rxjs/_esm2015/internal/operators/mergeMap.js:44:1)
      at MergeMapSubscriber._next (http://localhost:9876/_karma_webpack_/webpack:/node_modules/rxjs/_esm2015/internal/operators/mergeMap.js:34:1)
      at MergeMapSubscriber ...


Environment


Libs:
- @angular/core version: 12.6.16
- @ngxs/store version: 3.7.4 (when breaking)


Browser:
- Chrome Headless 100.0.4896.88 (Linux x86_64)
-  
For Tooling issues:
- Node version: 12.22.7
- Platform:  Linux, Debian

Others:

Downgrading from 3.7.4 to 3.7.3 resolved the issue

@willocho willocho changed the title 🐞[BUG]: 🐞[BUG]: 'Error: You have forgotten to import the NGXS module' after upgrading to 3.7.4 Jun 9, 2022
@arturovt arturovt self-assigned this Jun 11, 2022
@Frankitch
Copy link

Same problem here but also while serving angular.

@XardasLord
Copy link

The same happened to me with 3.7.4 with Angular 14.0.2

@GabrielAPineiro
Copy link

I got an close issue, but instead of forgotten to import NGXS module, I'm getting Injector has already been destroyed
Error: NG0205: Injector has already been destroyed.
error properties: Object({ code: 205 })
Error: NG0205: Injector has already been destroyed.
at R3Injector.assertNotDestroyed (node_modules/@angular/core/fesm2015/core.mjs:11360:1)
at R3Injector.get (node_modules/@angular/core/fesm2015/core.mjs:11288:1)
at localInject (node_modules/@ngxs/store/fesm2015/ngxs-store-internals.js:264:1)
at node_modules/@ngxs/store/fesm2015/ngxs-store.js:3927:54

@arturovt
Copy link
Member

arturovt commented Jun 21, 2022

@willocho are you able to install the 3.7.4-dev.master-310a613 version and give it a try? Just update all packages as follows:

"@ngxs/store": "3.7.4-dev.master-310a613",
"@ngxs/storage-plugin": "3.7.4-dev.master-310a613",
...

@danielrie
Copy link

@arturovt problem persists with the 3.7.4-dev.master-310a613 versions.

@jaibeales
Copy link

jaibeales commented Jun 22, 2022

I'm seeing the same You have forgotten to import the NGXS module' issue when running the application after upgrade. Oddly this works the first time when going to route that is subscribing to a slice of state via @select decorator, subsequent visits to same route fail.

I have tried the updated 3.7.4-dev.master-310a613 package and makes no difference.

Removing the the @select decorator and using the store directly fixes the issue, eg:

  //@Select(UserState) userState$: Observable<UserStateModel>;
  userState$ = this.store.select<UserStateModel>(UserState);

For further context this is in service scoped to a component. Eg (much simplified for brevity):

@Injectable() // NOTE: scope this to the page.
export class KanbanDataService implements OnDestroy {
  @Select(UserState) userState$: Observable<UserStateModel>;
   constructor(private store: Store){
      const sub = this.userSate$.subscribe(); // much simplified for brevity
      this.subscriptions.push(sub);
   }
  ngOnDestroy() {
    this.subscriptions.forEach(s => s.unsubscribe());
  }
}

@XardasLord
Copy link

Exactly the same scenario happens to me as for @jaibeales

@lehmstedt
Copy link

I don't know if it can help, but I encountered the same error in 3.7.3 and Angular 13.

The reason it failed is subscribing the selector resulting observable in the component constructor, instead of ngOnInit, in fact, not respecting the Angular component lifecycle.

@Select(BananaState.bananas) bananas$: Observable<Banana>;

// don't
constructor(){
  this.bananas$.subscribe(banana => doSometing());
}

//do
ngOnInit(){
  this.bananas$.subscribe(banana => doSometing());
}

@arturovt
Copy link
Member

After a lengthy discussion, we decided to deprecate the Select decorator since it has impossible bugs to fix (just because Angular itself doesn't allow it). We're also planning to drop it in the future. It's complicated to maintain since we have to invest a lot of time into a single Select decorator rather than focusing on other essential issues. It's not type-safe. It has problems in server-side rendering and module federation. We tried to fix the server-side rendering issue initially, leading to a dead end.

For now, start replacing the Select decorator with store.select. You also shouldn't have any issues with 3.7.4 if you drop Select decorator usages.

@ckapilla
Copy link

ckapilla commented Jul 4, 2022

First of all great thanks to @lehmstedt for describing what was the cause for me getting the error in about 5 - 6 of of 52 components using it in my app. I had just upgraded to angular 13 and was pretty distraught that everything was breaking left and right. Such a relief to be able to fix them all easily.

I don't look forward to replacing all those @Selects in the future, and I will miss using it. But I guess if it must be, it must be. When making this change, though, please give lots of examples of how to map different uses of @select to store.select. I found that the example above:
//@Select(UserState) userState$: Observable<UserStateModel>; userState$ = this.store.select<UserStateModel>(UserState);
didn't really work for me (given my limited understanding). Thanks

@arturovt
Copy link
Member

arturovt commented Jul 4, 2022

@ckapilla the @Select decorator uses store.select internally. Thus when you do @Select(UserState) it passes all arguments directly to store.select so this becomes store.select(UserState). The issue is it's tightened to a static Store instance; this hasn't been an issue in a single frontend application (w/o SSR and module federation).

SSR apps might be rendered concurrently when multiple HTTP requests are done and the second application being rendered overwrites the static Store property with its instance. This means the first application (which is also being rendered) uses the wrong Store instance (the second app created that). The same issue is with the module federation.

@digitalcraftco
Copy link

@ckapilla I did the same same thing at first based off of the docs but here is an example showing what @arturovt last comment explains. Avoid this.store.select<UserStateModel>(UserState);

Convert:
@Select(TransactionsState.transactions) transactions$!: Observable<TransactionData>;

To:
transactions$: Observable<TransactionData> = this.store.select(TransactionsState.transactions);

Then you can subscribe or async pipe etc.
this.transactions$.subscribe(data => console.log(data));

@evkw
Copy link

evkw commented Jul 12, 2022

After a lengthy discussion, we decided to deprecate the Select decorator since it has impossible bugs to fix (just because Angular itself doesn't allow it). We're also planning to drop it in the future. It's complicated to maintain since we have to invest a lot of time into a single Select decorator rather than focusing on other essential issues. It's not type-safe. It has problems in server-side rendering and module federation. We tried to fix the server-side rendering issue initially, leading to a dead end.

For now, start replacing the Select decorator with store.select. You also shouldn't have any issues with 3.7.4 if you drop Select decorator usages.

There goes my weekend 🤣

image

@ckapilla
Copy link

@ckapilla I did the same same thing at first based off of the docs but here is an example showing what @arturovt last comment explains. Avoid this.store.select<UserStateModel>(UserState);

Convert: @Select(TransactionsState.transactions) transactions$!: Observable<TransactionData>;

To: transactions$: Observable<TransactionData> = this.store.select(TransactionsState.transactions);

@digitalcraftco thanks so much for those details, looking forward to giving it a try.

@jaibeales
Copy link

Is there some confusion re Avoid this.store.select<UserStateModel>(UserState); ? Aren't we to avoid the @Select decorator and not the generic version of store.select<T>()?
Prob the best option is actually the select<T>(selector: StateToken<T>): Observable<T> for type safety.

@digitalcraftco
Copy link

digitalcraftco commented Jul 12, 2022

@jaibeales I think that makes the most sense. The confusion is arising from us who don't fully understand whats happening behind the scenes of the @Select decorator in the first place and what the correct (official) path is to convert them all over. I was running into a lot cannot read properties errors when trying convert @Select like below. Maybe you have some better insight for us?

animals$: Observable<string[]>; this.animals$ = this.store.select(state => state.zoo.animals);

someState$ = this.store.select<SomeStateModel>(SomeState);

@lehmstedt
Copy link

lehmstedt commented Jul 14, 2022

Small tip for changing the @select

In Vscode, you can regex @Select\((.*)\)\n(.*): Observable<(.*)>;$
and replace by $2 = this.store.select<$3>($1);

@johnhwright
Copy link

johnhwright commented Aug 2, 2022

We got this error in one component after upgrading but found it was due to a service that was being provided locally to a component in the providers array when it didn't need to be. Changing it to be global via the providedIn: 'root' option fixed it for now. We only have about ~100 uses of Select in our app, but since none of them are currently breaking on 3.7.4 will we need to worry about this change? Or is this just for applications that use SSR/universal?

@markwhitfeld
Copy link
Member

Our recommendation to move away from the decorator still stands but we decided to only introduce the deprecation message in v3.8 because we will be providing a cleaner alternative to the decorator that does not require injecting the store.
Keeping this issue open until this alternative and deprecation message is released.

@ckapilla
Copy link

ckapilla commented Aug 9, 2022

great news -- much appreciate your coming up with a cleaner solution

@markwhitfeld
Copy link
Member

Just for a bit of a preview of what we are looking at, here is a prototype I made last year:
https://stackblitz.com/edit/ngxs-select-dispatch-utils-v0-bmecrf?file=src%2Fapp%2Fapp.component.ts

It is in Angular 11, so there may be a small tweak around the inject function based on the Angular version you are using.
I have had some customers drop this utility in their codebase already and are loving it!
We are certain that we would be adding the select utility that you see here, potentially the dispatch utility (depending on how much feedback we get on the API for it) and most likely not the selectSnapshot utility (it is not particularly performant in its current form).

@jaibeales
Copy link

jaibeales commented Aug 12, 2022

Nice @markwhitfeld! Lovely simple functions ♥. I hadn't heard of ɵɵdirectiveInject(), using this from a function to get dependencies rather than injecting via constructor is very powerful (although somewhat experimental?).

@briancodes
Copy link

Would it be worth updating the select docs with a note on the deprecation, with the alternative approach highlighted e.g for people who aren't aware and start on a new project for the first time

@hakimio
Copy link

hakimio commented Feb 16, 2023

@markwhitfeld utility functions seem to work great in Angular 15 with inject() instead of private ɵɵdirectiveInject():
https://stackblitz.com/edit/ngxs-select-dispatch-utils-v0-fa8hnc?file=src%2Fapp%2Fapp.component.ts

@luishcastroc
Copy link

i see the first draft for the changes in version 3.8 so I think this is a work in progress and it soon will be resolved, thanks to the whole team that is putting a lot of effort on this.

@HarelM
Copy link

HarelM commented May 7, 2023

I'm trying to migrate to ngxs and my karma tests are experiencing this weird issue.
I've removed all @select from my services (I only test those ATM) and the issue is still there.
I can create a branch of my repo if it will help anyone, I don't think I can create a minimal reproduction as I have no clue what's causing this.
I'm using version 3.8, and @select is not deprecated there as far as I've seen. Is the plan to deprecate it still on motion?

Error: NG0205: Injector has already been destroyed.
error properties: Object({ code: 205 })
Error: NG0205: Injector has already been destroyed.

@markwhitfeld
Copy link
Member

@HarelM could you create a new issue and include a full reproduction?
Please include all required information in the issue template when you log it.
This issue looks like a different one.

@HarelM
Copy link

HarelM commented May 7, 2023

I think it was an error on my end unfortunately. I managed to fix the issue by making sure the test is executing the setTimeout I had on my service.
Thanks for the super quick response!
So this still leaves the question if @select will be deprecated in the future.
And another one, which is related to my issue, is if it's OK to dispatch from a @select subsription? In angular-redux which I currently use it creates a problem of state change fires incorrectly, so I needed to add a setTimeout. Is this needed here?

@markwhitfeld
Copy link
Member

markwhitfeld commented May 8, 2023 via email

@HarelM
Copy link

HarelM commented May 8, 2023

The main reason I'm asking is because I had a bug when using angular-redux with the same flow:
IsraelHikingMap/Site#1749 (comment)
I'm not sure I know how to solve this without updating the state from a state change event.
My flow is as follows:

  1. I get an event from the GPS position
  2. I store this location in the state (so it can be shared and subscribed to)
  3. I listen to location state changes in order to add it to a recording line -> which updates the state with the recorded line

I do all the logic outside the state classes and leave them to be simple, without logic, just update state from the action data, so I'm not sure how to avoid this "loop".

@kewur
Copy link

kewur commented Feb 4, 2024

are the alternatives included in 3.8 now or should we be using the ngxs-util that was provided earlier in this thread?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests