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

AOT and reducer factories in v4 #116

Closed
ValeryVS opened this issue Jul 19, 2017 · 35 comments
Closed

AOT and reducer factories in v4 #116

ValeryVS opened this issue Jul 19, 2017 · 35 comments
Assignees

Comments

@ValeryVS
Copy link

I'm experimenting with factories for ngrx.
Here is example app.
https://github.com/ValeryVS/angular-storage-experiment

with v2
https://github.com/ValeryVS/angular-storage-experiment/blob/16febfb4d141476dfefe97b5de4b8f90d318d68b/src/app/store/reducer.ts

with v4
https://github.com/ValeryVS/angular-storage-experiment/blob/46c20b987369b0017435f37fada4febdcf1e6937/src/app/store/reducer.ts

However, in v4 version I have AOT error.

ERROR in Error encountered resolving symbol values statically. Expression form not supported (position 16:7 in the original .ts file), resolving symbol createReducer in /Users/valery/Projects/storage-experiment/src/app/store/organization/reducers/index.ts, resolving symbol compileReducers in /Users/valery/Projects/storage-experiment/src/app/store/reducer.ts, resolving symbol AppModule in /Users/valery/Projects/storage-experiment/src/app/app.module.ts, resolving symbol AppModule in /Users/valery/Projects/storage-experiment/src/app/app.module.ts, resolving symbol AppModule in /Users/valery/Projects/storage-experiment/src/app/app.module.ts

Can you give an advice, what to change?

@karptonite
Copy link
Contributor

karptonite commented Jul 19, 2017

I can confirm that I am having similar issues. Here is relevant code in my project:
https://gist.github.com/karptonite/807150583a086ed8736f129d8094e88c
my error is different, though:
ERROR in Error encountered resolving symbol values statically. Function calls are not supported. Consider replacing the function or lambda with a reference to an exported function (position 5:9 in the original .ts file), resolving symbol createNamedWrapperReducer in /Users/karp/Documents/git/bgg/geekui2/src/app/store/meta/meta-reducers.functions.ts, resolving symbol createSubscriptionStatusesReducer in /Users/karp/Documents/git/bgg/geekui2/src/app/store/subscriptions/subscription-statuses.reducer.ts, resolving symbol reducers in /Users/karp/Documents/git/bgg/geekui2/src/app/store/reducers.ts, resolving symbol AppModule in /Users/karp/Documents/git/bgg/geekui2/src/app/app.module.ts, resolving symbol AppModule in /Users/karp/Documents/git/bgg/geekui2/src/app/app.module.ts, resolving symbol AppModule in /Users/karp/Documents/git/bgg/geekui2/src/app/app.module.ts

I haven't dug too much into it yet.
I'll add that, bizarrely, if I do my compilation automatically with ng serve, I get this error the first time it compiles, but any time it recompiles, it compiles successfully.

@shlomiassaf
Copy link

That's a limitation from the angular AOT compiler.

It is partially explained here

However, that explanation refers to the metaReducers part.

The problem you'r having is with the reducers argument.

This workaround should probably work:

export const reducers: ActionReducerMap<RootState> = {};
reducers.organization = fromOrganization.createReducer(ORGANIZATION);
reducers.user = fromUser.createReducer(USER);

The team will probably need to support a reducer factory function for reducers but that might confuse dev's that will send a reducer function to it.

I did see that there is a reducerFactory property in the options which might just do that but its not documented and I didn't follow the code.

@karptonite
Copy link
Contributor

karptonite commented Jul 19, 2017

@shlomiassaf That sort of worked--thanks. I had to remove the equivalent of organizations and users from the interface of RootState, however, or typescript would not accept the initial assignment.

It is a weird bug all round, especially the fact that compilation didn't always seem to fail.

@brandonroberts
Copy link
Member

You can use an Injection token instead. See ngrx/store#444 (comment)

@karptonite
Copy link
Contributor

@brandonroberts ah, that looks cleaner! I'll give it a try.

@alexjlockwood
Copy link

Has anyone tested whether the example app in this repo works with AOT?

@shlomiassaf
Copy link

I did not check but I can't get this to work....

The ReducerManager get's the InjectonToken injected instead of the reducers, in AOT.
In JIT it works fine,.

@alexjlockwood
Copy link

I have not tested either (trying to figure out how to build it right now) but I noticed this line of code in the app module:

@NgModule({
  imports: [
    // ...

    !environment.production ? StoreDevtoolsModule.instrument() : [],
 
    // ...
  ],
  bootstrap: [AppComponent],
})
export class AppModule {}

I forget exactly what the reasoning was, but I remember debugging my code for hours trying to figure out why my app was building fine w/o AOT but was failing with the AOT flag enabled. I finally realized that my code was failing to build because I was dynamically enabling/disabling certain imports in my NgModule decorator depending on the value of the environment.production flag. So this code looks very suspicious to me.

@shlomiassaf
Copy link

shlomiassaf commented Jul 19, 2017

Ok, ran this repo AOT.

I can confirm that the using an InjectionToken in the reducers does not work

To run the example app AOT you need to:

  • Clone the repo
  • instal node modules.
  • add and install the packages
yarn add @ngrx/store @ngrx/effects @ngrx/router-store @ngrx/store-devtools

Yes... we need to do that so we wont need to get crazy with AOT path mapping for the modules.

  • Remove the path mappings from tsconfig
  • run `ng build -prod -aot

cc @brandonroberts

@alexjlockwood
Copy link

Also, not sure if this has already been mentioned, but there are some docs on AOT compilation with v4 here: https://github.com/ngrx/platform/blob/master/docs/store/api.md#initial-state-and-ahead-of-time-aot-compilation

@shlomiassaf
Copy link

This looks like heavy DI stuff.

At the point where the providers needs to resolve the user provided token for the reducers is not present, kind of...

It is present (with its value) in the module definition and the providers for the module definition.
But it is not present on the injector.

It will be but not at this point, I tried injection it into the constructor of the AppRootModule and it worked...

image

The picture above is that moment when StoreModule needs to resolve it.
To get there I replaced the providers definition in StoreModule for the INITIAL_REDUCERS

        { provide: _INITIAL_REDUCERS, useValue: reducers },
        {
          provide: INITIAL_REDUCERS,
          useFactory: _initialReducersFactory,
          deps: [_INITIAL_REDUCERS, Injector],
        },
export function _initialReducersFactory(reducers: any, injector: Injector): any {
  return reducers instanceof InjectionToken
    ? injector.get(reducers)
    : reducers
  ;
}

Put a breaking point at the function when it starts, and thats the state for that image

@shlomiassaf
Copy link

shlomiassaf commented Jul 20, 2017

Ok, with more digging into the problem i'm not sure it's a bug in ngrx

I managed to get this working using the fix I set in the last comment + a workaround.

This splits into multiple issues that together cause this.

Why the fix (_initialReducersFactory):

The AOT compiler does not handle metadata logic statement properly

The INITIAL_REDUCERS provider is defined like this:

reducers instanceof InjectionToken
          ? { provide: INITIAL_REDUCERS, useExisting: reducers }
          : { provide: INITIAL_REDUCERS, useValue: reducers },

The AOT compiler is aware of that since this gets reflected in the ngsummary.json files.
But it does not evaluate it in compilation (or its does, but not correct)

When we examine the AOT compiled code:

__WEBPACK_IMPORTED_MODULE_0__angular_core__["d"/* ɵmpd */](
  256, 
  __WEBPACK_IMPORTED_MODULE_5__ngrx_store__["l"/* INITIAL_REDUCERS */],
  __WEBPACK_IMPORTED_MODULE_1__app_app_module__["b"/* REDUCERS_TOKEN */],
  []
);

The above is the AOT representation of the logical statement above it.

Since reducers is static at compilation AOT can run that logical expression and act upon it...

The flag 256 is the result of the expression.

    TypeValueProvider = 256,
    TypeClassProvider = 512,
    TypeFactoryProvider = 1024,
    TypeUseExistingProvider = 2048,

It means that AOT yielded useValue and not useExisting...

I think we need firepower here, @mhevery @tbosch @robwormald

Workaround

The workaround (after applying the fix) is just making sure that the provider is available before StoreModule.

Setting the provider at the providers array of the AppModule is not good since it will not be available once the token is needed.

So just make sure it's evaluated before:

export const REDUCERS_TOKEN = new InjectionToken<ActionReducerMap<any>>('Registered Reducers');

@NgModule({
  providers: [
    { provide: REDUCERS_TOKEN, useValue: reducers }
  ]
})
export class TempModule {
  constructor(@Inject(REDUCERS_TOKEN) r: any) {
    console.log(r);
  }
}

And then set this module in the AppRoot imports.

Its important to inject it in the constructor else it won't eval (its lazy)

@shlomiassaf
Copy link

Workaround until this is resolved:

export const reducerToken = new InjectionToken<ActionReducerMap<State>>('Registered Reducers');

export const reducerProvider = [
  { provide: reducerToken, useValue: reducers }
];

After creating the token and provider act as if the token is the reducer map

Object.assign(reducerToken, reducers);

Will do the job for now.

@ValeryVS
Copy link
Author

Thanks for the workarounds.
I can confirm that both mentioned are work.
One with InjectionToken looks more accurate, because we not loose benefits of ActionReducerMap typings.

With InjectionToken:
https://github.com/ValeryVS/angular-storage-experiment/blob/fix/ngrx-reducer-factory-aot-v2/src/app/store/reducer.ts

With object properties moving:
https://github.com/ValeryVS/angular-storage-experiment/blob/fix/ngrx-reducer-factory-aot-v1/src/app/store/reducer.ts

@karptonite
Copy link
Contributor

FYI, the InjectorToken workaround currently only works for reducers that are imported in forRoot, not forFeature. see #141.

@shlomiassaf
Copy link

shlomiassaf commented Jul 20, 2017

Want to emphasise that this is a "hack", tomorrow the angular team can decide to "freeze" the injection token instance... who knows... so this should be fixed.

@karptonite
Copy link
Contributor

Hmm... in that case, should we implement the use of InjectionToken for forFeature, or is there some other way around this problem that would be less hacky?

@shlomiassaf
Copy link

Well, that's not much of a worry for now...

Using the workaround (object.assign(reducerToken, reducers);) is simple and transparent since it does not change the logic.

Once this issue is fixed you just remove the object.assign and that's it you're good.

Assuming they won't change the implementation, which we should expect...

@karptonite
Copy link
Contributor

@shlomiassaf I'm not sure I understand the Object.assign solution. Where exactly are you calling Object.assign?

@shlomiassaf
Copy link

When using @brandonroberts suggestion to use DI

export const reducerToken = new InjectionToken<ActionReducerMap<State>>('Registered Reducers');

export const reducerProvider = [
  { provide: reducerToken, useValue: reducers }
];

We face an issue, it does not work on AOT.

What happens in AOT is that due to a bug the reducers object does not inject, instead the injected object is reducerToken (again, bug)

There's no easy way to fix that without starting to change the whole logic...
so, when I object.assign(reducerToken, reducers) i'm just taking the reducer map and "coping" it to the token... since that token is thought to be the reducer map because of the bug mentioned above it actually works...

@karptonite
Copy link
Contributor

@shlomiassaf OK, I still don't understand. Where are you calling Object.assign? what does your @ngModule look like with this approach? what are you passing to StoreModule.forRoot (or forFeature)?

@shlomiassaf
Copy link

shlomiassaf commented Jul 20, 2017

export const REDUCERS_TOKEN = new InjectionToken<ActionReducerMap<State>>('Registered Reducers');

const reducers = {
  auth: () => {},
  nav: () => {},
  ...
}

// WORKAROUND HERE 
object.assign(REDUCERS_TOKEN, reducers)
// WORKAROUND HERE 

@NgModule({
  import: [
    StoreModule.forRoot(REDUCERS_TOKEN),
  ],
  providers: [
    { provide: REDUCERS_TOKEN, useValue: reducers }
  ]
})
export class AppModule { }

@karptonite
Copy link
Contributor

Thank you. Sorry if I seemed obtuse. It seems to work, although I haven't tested with AOT.

@shlomiassaf
Copy link

shlomiassaf commented Jul 20, 2017

Sure, all good.

I did test with AOT, for me it works.

FYI: With JIT the object.assign(REDUCERS_TOKEN, reducers) has no meaning, only with AOT.
Also works for @ValeryVS

@ValeryVS
Copy link
Author

@shlomiassaf Is Object.assign required?
I test InjectionToken workaround in two projects and in seems to just work with AOT.

I define reducers, reducerToken and reducerProvider in reducers.ts file.
It is important, that reducers must also be exported. Not only token and provider.
https://github.com/ValeryVS/angular-storage-experiment/blob/fix/ngrx-reducer-factory-aot-v2/src/app/store/reducer.ts

@ValeryVS
Copy link
Author

If i understand it right:

if provided value is InjectorToken, then it will be provided as INITIAL_REDUCERS without changes.

reducers instanceof InjectionToken

else INITIAL_REDUCERS will be created with this constructor, which is, again, InjectionToken.

export const INITIAL_REDUCERS = new InjectionToken(

@maxisam
Copy link
Contributor

maxisam commented Jul 21, 2017

@ValeryVS @shlomiassaf For AOT, I need Object.assign in my case as well. Just in case, I am on Angular 4.3.1. @ngrx/store 4.0.0.

Without that line, ngrx/store doesn't get the reducer object.

@maxisam
Copy link
Contributor

maxisam commented Jul 22, 2017

@ValeryVS I tried your repo. It doesn't work with ng serve- aot if I don't put Object.assign hack.

@jogboms
Copy link

jogboms commented Jul 23, 2017

@shlomiassaf Thanks for the hack but from my end, @Effects don't seem to pick up actions.

@derekkite
Copy link

I used metareducers based on this:
https://github.com/btroncone/ngrx-examples/blob/master/todos-undo-redo/src/app/reducers/undoable.ts

and was getting AOT errors when I ported to v4. Injecting with the Object.assign hack works, as well as the effects. Didn't work without the Object.assign.

@brandonroberts
Copy link
Member

Another option besides injecting a reducer token if you're using some type of factory function is to wrap that factory in an exported function. Example from opening post below. I'll close this with a PR to the docs about reducer factories.

export function organizationReducer(state, action): fromOrganization.State {
  return fromOrganization.createReducer(ORGANIZATION)(state, action);
}

export function userReducer(state, action): fromUser.State {
  return fromUser.createReducer(USER)(state, action);
}

export const reducers: ActionReducerMap<RootState> = {
  organization: organizationReducer,
  user: userReducer,
};

MikeRyanDev pushed a commit that referenced this issue Jul 25, 2017
Fixes AOT issues when providing reducers via an InjectionToken

Closes #116, #141, #147
@ValeryVS
Copy link
Author

@shlomiassaf @maxisam
Thanks for investigating. It definitely was not work without Object.assign.
https://github.com/ValeryVS/angular-storage-experiment/tree/ngrx%404-aot-fix-v2-1

@brandonroberts
Approach with wrappers works, but we need some kind of memoriser if factory then. Because it will be called with every action for all reducers, created with that factory.
If you recommend this way, I think it should me mentioned.

@MikeRyanDev @brandonroberts
I don't sure, that InjectionToken usage is completely fixed with #153.
It supports token usage for features, but we still need to add Object.assign hack.

Also, while app is compiled with AOT and @ngrx nightly builds, there is some runtime error.

Uncaught TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at combineReducers (store.es5.js:29)
    at store.es5.js:82
    at new ReducerManager (store.es5.js:179)
    at _createClass (core.es5.js:9526)
    at _createProviderInstance$1 (core.es5.js:9484)
    at initNgModule (core.es5.js:9438)
    at new NgModuleRef_ (core.es5.js:10546)
    at createNgModuleRef (core.es5.js:10530)
    at Object.debugCreateNgModuleRef [as createNgModuleRef] (core.es5.js:12824)

reducers is null at at combineReducers (store.es5.js:29)
https://github.com/ValeryVS/angular-storage-experiment/tree/02fd926e17547abfdb213b3d8dd3af3392218238

@shlomiassaf
Copy link

@ValeryVS I don't know how it worked for you before @brandonroberts fix, it strange since the AOT compilation will not allow that.

After the fix it should work without Object.assign but still you need to make sure that the provider is eager loaded.

@ValeryVS
Copy link
Author

It is, probably, some other bug. But because of mentioned in previous post Cannot convert undefined or null to object error, I can't test how it works.
Compilation is fine.
This error happens at runtime.

@JozefFlakus
Copy link

JozefFlakus commented Aug 2, 2017

I had this Cannot convert undefined or null to object error with @ngrx/core v4.0.2 with AOT compiled build. I locked the version to v4.0.0 and the errors gone away (Angular v.4.3.3). Didn't tested with feature stores.

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

10 participants