ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
7.99k stars 1.96k forks source link

No Provider issue on lazy loaded module and ViewContainerRef with Angular 14 #3700

Open Julienbideau opened 1 year ago

Julienbideau commented 1 year ago

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

router-store

Minimal reproduction of the bug/regression with instructions

Since we bumped versions from Angular V13 to V14, and ngrx 13.0 to 14.2, we have an error : NullInjectorError: NullInjectorError: No provider for Store!

We need to declare our Store in a lazy loaded module :

StoreModule.forRoot({}),
EffectsModule.forRoot([])

We also don't use routing in our app, just component initialization like so :

 @ViewChild('transactionContainer', { read: ViewContainerRef, static: true })
  public transactionContainer: ViewContainerRef | undefined;

  constructor(private readonly injector: Injector) {}

  async loadLazyComponent() {

    const lazyStoreModuleNgModuleRef = createNgModule(
      LazyStoreModule,
      this.injector
    );
    this.transactionContainer!.createComponent(LazyStoreComponent, {
      ngModuleRef: lazyStoreModuleNgModuleRef,
    });
  }

If I declare the store in the app.module.ts, It solves the problem but we can't declare the store there because we need our stores to be isolated in each component

I made a stackblitz : https://stackblitz.com/edit/angular-ivy-u9b5yo?file=package.json

If you downgrade to 13.2.0, we have no issues anymore.

Minimal reproduction of the bug/regression with instructions

The store to be correctly provided

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: 14.3.2 Angular 14.2.12 Node 16

Other information

Thank you in advance ! :-)

I would be willing to submit a PR to fix this issue

Phusty commented 1 year ago

We have the same problem in our project. Are there any news?

rudobri commented 1 year ago

Same Problem on our side. Please let us know. Thanks

Julienbideau commented 1 year ago

I tryed the 15.1.0 version and we still have the same problem

Simon-IEEECS commented 1 year ago

Just ran into this as well. Anyone solve it yet?

brandonroberts commented 1 year ago

We did not change anything with how injection works between those versions so it could have been a change in Angular. Will look into it though

vavon92 commented 1 year ago

We experienced this issue with the 14.3.0 version, we solved it by downgrading to the 14.2.0 version. Maybe it could be related to the change on how the services are provided. Hope it helps!

brandonroberts commented 1 year ago

Thanks @vavon92. That seems like a reasonable workaround for now. In 14.3.x, we did change where the providers are not inlined directly in the providers array to add support for standalone provider APIs, but to me, that should not have caused a breakage here.

markostanimirovic commented 1 year ago

Indeed, a breaking change was introduced in v14.3 in case root store providers are not provided at the root injector level (in AppModule or another eagerly loaded module).

To fix this issue, move the StoreModule.forRoot call to AppModule.imports: https://stackblitz.com/edit/angular-ivy-den6xd?file=src%2Fapp%2Fapp.module.ts

Simon-IEEECS commented 1 year ago

That's a temporary fix right? The whole point of lazy loading is that I don't need the store in my other modules

vavon92 commented 1 year ago

That's a temporary fix right? The whole point of lazy loading is that I don't need the store in my other modules

I hope so, we can't update to the v15 otherwise! It's quite blocking on our side as we will update to angular 15 soon (requiring v15 for ngrx)

Julienbideau commented 1 year ago

@vavon92 I just tried to downgrade to 14.2.0 in my stackblitz and I still have the issue

vavon92 commented 1 year ago

@Julienbideau We tested in on our production environment and it works, I don't have any more insights on that sorry Maybe you could try downgrading to the 14.0.0 and see if that works out for you

benj0c commented 1 year ago

@Julienbideau Checkout your package-lock.json file, make sure you have installed 14.2.0 version.

If you run ng update it will install 14.3.3 version, I fixed this with npm install instead.

bryanforbes commented 1 year ago

I have a very large application that I'm going to incrementally move over to the new Angular paradigms and I'm running into this same issue. I have some common state shared between lazy loaded features and I declare that via imports in the application NgModule using StoreModule.forRoot(). Then each older lazy loaded feature uses StoreModule.forChild() which works fine. I started creating a new feature module today using standalone components, routes with providers, and provideState() and as soon as I navigate to that route, I get the same error as above: NullInjectorError: NullInjectorError: No provider for InjectionToken @ngrx/store Root Store Provider!.

My app is using Angular 14 and ngrx 14, but I was able to put together a small-ish reproduction to demonstrate this is still happening in Angular 15 and ngrx 15:

https://stackblitz.com/edit/angular-wzciju

Julienbideau commented 1 year ago

I guess the issue comes from the new provide_store.ts added in 14.3.0 https://github.com/ngrx/platform/compare/14.2.0...14.3.0

I didn't found the issue yet

bryanforbes commented 1 year ago

@Julienbideau I haven't either. I suspect it's a race condition somewhere.

brandonroberts commented 1 year ago

@bryanforbes It's not a race condition. In your case, you're mixing two different ways of registering the Store. provideStore has providers that StoreModule.forRoot() does not have.

This works

import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { BrowserModule } from '@angular/platform-browser';
import { provideStore, StoreModule } from '@ngrx/store';
import { AppRoutingModule } from './app-routing.module';
import { AppComponent } from './app.component';
import { commonReducer, commonFeatureName } from './common';

@NgModule({
  declarations: [AppComponent],
  imports: [
    CommonModule,
    BrowserModule,
    AppRoutingModule,
    // StoreModule.forRoot({ [commonFeatureName]: commonReducer }), <-- Does not work with provideState()
  ],
  providers: [
    provideStore({ [commonFeatureName]: commonReducer }), // works with provideState()
  ],
  bootstrap: [AppComponent],
})
export class AppModule {}
bryanforbes commented 1 year ago

@brandonroberts Thank you for the quick response! Should StoreModule.forFeature() work with provideStore() at the root level? I have several feature modules that use StoreModule.forFeature() and I'm trying to avoid rewriting older code (I'd like to do it incrementally when I have time) as well as avoiding providing two stores.

brandonroberts commented 1 year ago

@bryanforbes yes, it should work also. provideStore() and includes all the previous providers and tokens, and a few new ones to support environment providers.

bryanforbes commented 1 year ago

@brandonroberts I forked my previous stackblitz, added some feature state to hello.module.ts, switched to using provideStore() at the root, and I'm now getting an injector error when loading the hello route: NullInjectorError: NullInjectorError: No provider for StoreRootModule

https://stackblitz.com/edit/angular-9lezro

brandonroberts commented 1 year ago

@bryanforbes ahh yes, that's correct because of the NgModules. You can put both in your AppModule, all the providers will get merged, so last one wins and you'll get all the necessary dependencies. The state only needs to be registered once though in provideStore().

@NgModule({
  declarations: [AppComponent],
  imports: [
    CommonModule,
    BrowserModule,
    AppRoutingModule,
    StoreModule.forRoot(),
  ],
  providers: [provideStore({ [commonFeatureName]: commonReducer })],
  bootstrap: [AppComponent],
})
export class AppModule {}
bryanforbes commented 1 year ago

@brandonroberts Perfect! Thanks for helping me debug that. It might be good to add that as an FAQ in the docs.

bryanforbes commented 1 year ago

@brandonroberts will I need to do something similar with EffectsModule.forRoot() and provideEffects() in the app module?

brandonroberts commented 1 year ago

@bryanforbes most likely yes, same rule applies though with only registering the root effects once if you have any

wahajahmedkhan commented 1 year ago

A breaking change was introduced in v14.3. I am still curious how come it was working in the early version because it shouldn't. It is recommended to only import StoreModule.forRoot({}) in the root module of your Angular application, not in a lazy-loaded module. This is because StoreModule.forRoot({}) registers the store singleton with the root injector, and you don't want to create multiple instances of the store in your application.

if you want to create a lazy loaded store module for the feature.

Add the StoreModule.forFeature() and EffectsModule.forFeature() methods to the imports array of the new module. For example, you can add the following:

imports: [
  CommonModule,
  StoreModule.forFeature('books', booksReducer),
  EffectsModule.forFeature([BooksEffects]),
],

A fix to this issue would be https://github.com/ngrx/platform/issues/3700#issuecomment-1397683026

Alex85651 commented 1 year ago

Any news about this issue ?

Achilles1515 commented 11 months ago

@brandonroberts does a solution exist for actual question posed by the OP? For example, we have a host app that lazy loads a remote app using module federation.

The host app does not use NgRx. The remote app does use NgRx.

The goal is for the host app to not know or even care what the remote app is doing for state management (context: remote app is written by a completely separate team working mostly independently).

But currently because of this NullInjectorError, we are forced to include NgRx in the host app and do the StoreModule.forRoot stuff in the AppModule. --> I really don't want this code in the host app. It is irrelevant to the host app and increases the bundle size of the host app, and may not even get used (i.e. remote module may not get fetched), depending on how the user interacts with the app.

I want this lazy loaded remote module to be completely in charge of it's state management concerns, without having to rely on the host app.

Is there any solution for this?

kuldeepGDI commented 11 months ago

Indeed, a breaking change was introduced in v14.3 in case root store providers are not provided at the root injector level (in AppModule or another eagerly loaded module).

To fix this issue, move the StoreModule.forRoot call to AppModule.imports: https://stackblitz.com/edit/angular-ivy-den6xd?file=src%2Fapp%2Fapp.module.ts

this workaround pretty much is not what one can do i think for Module federation use case where shell UI and module federated components are pretty much independent of each others.

scottbisaillon commented 10 months ago

@brandonroberts does a solution exist for actual question posed by the OP? For example, we have a host app that lazy loads a remote app using module federation.

The host app does not use NgRx. The remote app does use NgRx.

The goal is for the host app to not know or even care what the remote app is doing for state management (context: remote app is written by a completely separate team working mostly independently).

But currently because of this NullInjectorError, we are forced to include NgRx in the host app and do the StoreModule.forRoot stuff in the AppModule. --> I really don't want this code in the host app. It is irrelevant to the host app and increases the bundle size of the host app, and may not even get used (i.e. remote module may not get fetched), depending on how the user interacts with the app.

I want this lazy loaded remote module to be completely in charge of it's state management concerns, without having to rely on the host app.

Is there any solution for this?

Even when including StoreModule.forRoot() in the host's app module, we still see the same issue when using module federation. What does your webpack.config look like in regards to the shared libs?

Achilles1515 commented 10 months ago

Even when including StoreModule.forRoot() in the host's app module, we still see the same issue when using module federation. What does your webpack.config look like in regards to the shared libs?

Like this:

let federatedModules = withModuleFederationPlugin({
    shared: share({
       // angular, rxjs, others

        '@ngrx/store': { singleton: true, eager: true, strictVersion: true },
        '@ngrx/effects': { singleton: true, eager: true, strictVersion: true },
    }),
    sharedMappings: [],
});

I believe the remote looks similar, maybe without eager though.

brandonroberts commented 10 months ago

Thanks, everyone. We'll take a look, but we're not committing to any changes. The support for module federation in this way should be opened as a separate issue.

Achilles1515 commented 10 months ago

@brandonroberts I was using module federation as a personal example, but the problem there is the exact same as the issue stated in the OP of this thread. The desire boils down to:

I want the app root injector to know knothing about NgRx (at least from the standpoint of putting NgRx code in app.module.ts file), and I instead want the NgRx store, effects, etc. to be initialized in a child module --> and therefore, to only be available for DI in this child modules' injector and downwards for their descendents.

Forgive me if I am misunderstanding something here, but NgRx is just a state management library to be utilized via DI...if you think of NgModules and their injectors as a tree, shouldn't you just be able to pick any NgModule in the tree and say "this is the root of an NgRx store, available for this module and its children"? I don't understand why the actual app root injector needs to know anything about that decision.

Anyway, as @wahajahmedkhan stated, this breaking change occurred starting in v14.3.0. And the problem is in the @ngrx/effects module. Multiple injectable classes (EffectsRunner, EffectSources, Actions) were changed to be provided in the root injector instead of module-level injector --> but the same type of change wasn't done for Store.

// @Injectable()  <--- USED TO BE THIS
@Injectable({ providedIn: 'root' })    // <--- BUT NOW EXPLICITLY IN ROOT
export class EffectsRunner implements OnDestroy {
  //...

  constructor(
    private effectSources: EffectSources,
    private store: Store<any>
  ) {}

  //...
}

Since the above class is created in the root injector, its constructor needs a Store coming from the root injector (and again, the Store is residing on the module-level wherever StoreModule.forRoot was called).

In this StackBlitz forked from @Julienbideau 's example, I added the [applicable] ngrx@14.3.0 source code and fixed the problem by taking out the injectable { providedIn: 'root' } options for the classes I stated above, and then adding them as returned providers in the EffectsModule.forRoot method.

I don't know the history behind why { providedIn: 'root' } was added, but can you look into removing them so that the Store and Effects services stay on the same module-level?

Julienbideau commented 10 months ago

@brandonroberts I was using module federation as a personal example, but the problem there is the exact same as the issue stated in the OP of this thread. The desire boils down to:

I want the app root injector to know knothing about NgRx (at least from the standpoint of putting NgRx code in app.module.ts file), and I instead want the NgRx store, effects, etc. to be initialized in a child module --> and therefore, to only be available for DI in this child modules' injector and downwards for their descendents.

Forgive me if I am misunderstanding something here, but NgRx is just a state management library to be utilized via DI...if you think of NgModules and their injectors as a tree, shouldn't you just be able to pick any NgModule in the tree and say "this is the root of an NgRx store, available for this module and its children"? I don't understand why the actual app root injector needs to know anything about that decision.

Anyway, as @wahajahmedkhan stated, this breaking change occurred starting in v14.3.0. And the problem is in the @ngrx/effects module. Multiple injectable classes (EffectsRunner, EffectSources, Actions) were changed to be provided in the root injector instead of module-level injector --> but the same type of change wasn't done for Store.

// @Injectable()  <--- USED TO BE THIS
@Injectable({ providedIn: 'root' })    // <--- BUT NOW EXPLICITLY IN ROOT
export class EffectsRunner implements OnDestroy {
  //...

  constructor(
    private effectSources: EffectSources,
    private store: Store<any>
  ) {}

  //...
}

Since the above class is created in the root injector, its constructor needs a Store coming from the root injector (and again, the Store is residing on the module-level wherever StoreModule.forRoot was called).

In this StackBlitz forked from @Julienbideau 's example, I added the [applicable] ngrx@14.3.0 source code and fixed the problem by taking out the injectable { providedIn: 'root' } options for the classes I stated above, and then adding them as returned providers in the EffectsModule.forRoot method.

I don't know the history behind why { providedIn: 'root' } was added, but can you look into removing them so that the Store and Effects services stay on the same module-level?

Hey Achille ! Interesting answer, do you think it would be possible to override some ngrx internal sources in our project in order to do the upgrade to ngrx 16? Just in order to wait till a fix

Achilles1515 commented 10 months ago

@Julienbideau It took me so long today to come up with a hack to get it working....but I think I got it. Here is a fork of your StackBlitz, with everything upgraded to latest Angular 16.

All modifications to get it working are in the lazy.store.module.ts file.

// Workaround for the following issue:
// https://github.com/ngrx/platform/issues/3700
import { EffectSources, EffectsRunner, Actions  } from '@ngrx/effects';

const effectProvidersForWorkaround = [
  EffectsRunner,
  EffectSources,
  Actions,
];
effectProvidersForWorkaround.forEach(p => p.ɵprov.providedIn = null);

and then passing the effectProvidersForWorkaround into the EffectsModule.forRoot call like this:

StoreModule.forRoot({}),
EffectsModule.forRoot(effectProvidersForWorkaround),

I've never actually used NgRx so I can't say with complete certainty everything works as expected, but it resolves the missing providers errors. Let me know if it works for you.

carlokid commented 9 months ago

@brandonroberts does a solution exist for actual question posed by the OP? For example, we have a host app that lazy loads a remote app using module federation.

The host app does not use NgRx. The remote app does use NgRx.

The goal is for the host app to not know or even care what the remote app is doing for state management (context: remote app is written by a completely separate team working mostly independently).

But currently because of this NullInjectorError, we are forced to include NgRx in the host app and do the StoreModule.forRoot stuff in the AppModule. --> I really don't want this code in the host app. It is irrelevant to the host app and increases the bundle size of the host app, and may not even get used (i.e. remote module may not get fetched), depending on how the user interacts with the app.

I want this lazy loaded remote module to be completely in charge of it's state management concerns, without having to rely on the host app.

Is there any solution for this?

This is also happening to me, host must install ngrx in order to accommodate the MFE with ngrx. Additionally, I also have to modify the selectors, since I modified the exposed remote module to use forFeature, where it works previously with forRoot. The MFE project that I am working on is also a standalone app, that's why they should both work with the same selectors, so I have to modify the app.module as well to use forFeature.

Simon-IEEECS commented 9 months ago

@brandonroberts Can the team assign someone to this issue?

brandonroberts commented 9 months ago

Have a WIP solution here https://github.com/ngrx/platform/pull/4119 but trying to see what impact it has on standalone APIs.

Alex85651 commented 9 months ago

@Julienbideau It took me so long today to come up with a hack to get it working....but I think I got it. Here is a fork of your StackBlitz, with everything upgraded to latest Angular 16.

All modifications to get it working are in the lazy.store.module.ts file.

// Workaround for the following issue:
// https://github.com/ngrx/platform/issues/3700
import { EffectSources, EffectsRunner, Actions  } from '@ngrx/effects';

const effectProvidersForWorkaround = [
  EffectsRunner,
  EffectSources,
  Actions,
];
effectProvidersForWorkaround.forEach(p => p.ɵprov.providedIn = null);

and then passing the effectProvidersForWorkaround into the EffectsModule.forRoot call like this:

StoreModule.forRoot({}),
EffectsModule.forRoot(effectProvidersForWorkaround),

I've never actually used NgRx so I can't say with complete certainty everything works as expected, but it resolves the missing providers errors. Let me know if it works for you.

Your workaround worked for me, thanks !

johanndev commented 7 months ago

The workaround provided by @Achilles1515 did not work in my module federation project. I had to revert to using the Module-based registration functions (StoreModule.forRoot() etc).

Tavisca-akshay-rajput commented 5 months ago

Any updates on this issue?

Simon-IEEECS commented 3 months ago

How are big new features getting worked on daily, yet a critical bug that's broken countless repositories STILL has no answer?

rainerhahnekamp commented 3 months ago

Hey @Simon-IEEECS, this is not an easy issue to solve. From what I can tell, you would have to potentially change significant parts of NgRx's inner workings.

Is it really worth the time and effort? In the end, we are discussing here the possibility to run provideStore() lazily. The fix would be to do in your host/shell.

How many KBytes in terms of optimized bundle size are we talking here about? If we have an empty store in root, it should be neglectable small. Especially considering a Micro Frontend architecture where you might have untreeshakable, shared dependencies.

As far as I see it, that is the only reason for that requested change. Do you see further issues?

(Don't see my answer as impolite, but I really want to understand the gravity of the problem)

mikezks commented 3 months ago

@Simon-IEEECS

I feel your pain. You can use the following code to try out using several Effect instances in parallel w/i the same Angular version. The Store part should work w/o any changes. Let me know whether that approach works in your environment.

export function provideEffectsLazy(
  effects: Array<Type<unknown> | Record<string, FunctionalEffect>>
): EnvironmentProviders;
export function provideEffectsLazy(
  ...effects: Array<Type<unknown> | Record<string, FunctionalEffect>>
): EnvironmentProviders;
export function provideEffectsLazy(...effects:
  | Array<Type<unknown> | Record<string, FunctionalEffect>>
  | [Array<Type<unknown> | Record<string, FunctionalEffect>>]
): EnvironmentProviders {
  const effectsClassesAndRecords = effects.flat();

  return makeEnvironmentProviders([
    Actions,
    EffectsRunner,
    EffectSources,
    {
      provide: EFFECTS_ERROR_HANDLER,
      useFactory: () => defaultEffectsErrorHandler
    },
    provideEffects(effectsClassesAndRecords),
  ]);
}

@ all Feel free to mention your MFE aspects as I am working together with @manfredsteyer on the Module Federation and Native Federation story for Angular. We are happy to support in this area.

To the NgRx Core Team: Please let us discuss whether you see any downsides in adding a similar concept like for provideStore(), provideState(). Would that tackle your concerns discussed in https://github.com/ngrx/platform/pull/4119 if we use provideEffectsLazy() to add a new set of providers for an independent scope and provideEffects() to connect new Effect implementations in child injector nodes? That way, we could support this niche requirement w/o breaking change.

EDIT: An even nicer signature would be: provideEffects(MyEffects, { createNewProviderScope: true }) That way, we would not even need a new provider fn and still no breaking change because of the optional EffectOptions.

Reach out, if you need a PR for that.

SimonStiph commented 2 months ago

Thanks for the explanation @rainerhahnekamp and your input @mikezks. Very good info. from both. Regarding the size of the bundle increase, it is certainly small. I'll try out the above remedy while the loading pattern gets sorted out