feat(store): support meta reducer factory for feature (#1414)

[itayod]

PR Checklist

Please check if your PR fulfills the following requirements:

• [ x] The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #1414

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ x] No

Other information

[ngrxbot]

Preview docs changes for a51d037 at https://previews.ngrx.io/pr1445-a51d037/

[coveralls]

Coverage increased (+0.006%) to 88.347% when pulling 3d8b50c on itayod:feature/meta-reducer-factory into 4ed16cd on ngrx:master.

[itayod]

please let me know if you like the solution...
if so I will add some test and update the pull request.

A code example of how to use this feature:

import {NgModule, InjectionToken} from '@angular/core';
import {StoreModule} from '@ngrx/store';

export const FEATURE_META_REDUCER_TOKEN = new InjectionToken('Feature Meta Reducers');

export function getMetaReducers() {
  // array of meta reducers
  return [function(reducer) {
    return function (state, action) {
      return reducer(state, action);
    }
  }];
}

@NgModule({
  imports: [
    StoreModule.forFeature('feature', {}, {metaReducers: FEATURE_META_REDUCER_TOKEN}),
  ],
  providers: [
    {
      provide: FEATURE_META_REDUCER_TOKEN,
      useFactory: getMetaReducers,
    },
  ],
})
export class FeatureModule { }

[timdeschryver]

I think the implementation is good and should solve the issue you've encountered.
What are your thoughts about this @brandonroberts , should we go further with this?
This change should be backward compatible with the option to override the metaReducers option config, by using an injection token.

[ngrxbot]

Preview docs changes for d6f7348 at https://previews.ngrx.io/pr1445-d6f7348/

[brandonroberts]

@timdeschryver I think if we're going to allow an injection token, it should at the configuration level and not the meta-reducer level. It would prevent us from having an injection token for each thing you want to configure using DI.

[itayod]

@timdeschryver I think if we're going to allow an injection token, it should at the configuration level and not the meta-reducer level. It would prevent us from having an injection token for each thing you want to configure using DI.

Having that said,
My solution allow users to define their own injection token for meta reducers,
It is also be easy to extend it for the rest of the configuration fields (e.g initialState) by adding it to the
_createFeatureStore function.

But of course it is for you to decide rather you like the idea or not.

All changes are made.

[timdeschryver]

So to clear, you mean something like this?
(Provided config properties are used to override the default properties)

export function _createFeatureStore(
  injector: Injector,
  configs: StoreFeature<any, any>[]
) {
  return configs.map(config => {
     return config instanceof InjectionToken ? {...config, ...injector.get(config)} : config;
  )}
}

[brandonroberts]

Yes, something like that. You wouldn't spread the config if its an InjectionToken though since it contains the entire feature object. We also need some tests to verify this correctly merges configs from different NgModules, including lazy loaded ones.

[itayod]

so the api will look something like that?

    export const MY_FEATURE_CONFIG_TOKEN = new InjectionToken('Feature Config');
    ...

    StoreModule.forFeature('feature', {}, MY_FEATURE_CONFIG_TOKEN),

[brandonroberts]

Yes

[itayod]

I have made some changes in another commit (just in case...)
@brandonroberts is that what you meant?

[brandonroberts]

Its getting there. I'd prefer to keep STORE_FEATURES as a multi-provider though. Tim already mentioned this, but I agree that using _STORE_FEATURES internally to resolve the feature configs is a better option.

There also needs to be a test that provides an injection token for a feature config.

[itayod]

I see, I think that if we must support multi provider for STORE_FEATURES there are 2 possible solutions,
1 is to change StoreFeatureModule constructor (which I am not sure if we want to...).
2 is to create a token for each of the StoreFeature feilds and provide them by use existing, which by the way will also allow users to create an injection token for a specific field (e.g the initial state, meta reducer, store key...) .

@brandonroberts @timdeschryver please let me know what do think, or if you have some other solutions, I will be happy to implement them :)

[timdeschryver]

I think the implementation is good, just a few details:

• based on feat(store): support meta reducer factory for feature (#1414) #1445 (comment), I think we want to "force" the user to provide all properties instead of a partial config. So the implementation would look something as:

export function _createFeatureStore(injector: Injector, features: Array<any>) {
  return features.map(feat => {

    if(feat.config instanceof InjectionToken) {
      const config = injector.get(feat.config);
      return {
        key: feat.key,
        reducerFactory: config.reducerFactory,
        metaReducers: config.metaReducers,
        initialState: config.initialState
      }
    }

    return {
      key: feat.key,
      reducerFactory: feat.config.reducerFactory
        ? feat.config.reducerFactory
        : combineReducers,
      metaReducers: feat.config.metaReducers ? feat.config.metaReducers : [],
      initialState: feat.config.initialState,
    };
  });
}

• I think we should keep supporting the multi-provider for STORE_FEATURES. If it stings to inject an internal token in the constructor, we could also make this new token public and rename it to STORE_FEATURES_COLLECTION (or STORE_FEATURES_CONFIG) ?

[brandonroberts]

There are a couple of options I see that we can do here.

1. Allow you to provide an InjectionToken for the StoreConfig, and fetch the config if needed in the constructor of the StoreFeatureModule where we are already mapping over feature.

2. Don't change anything and document using the ReducerManager to add features dynamically if you need dependency injection. We should do this anyway, but it may be a less risky solution.

[itayod]

@brandonroberts I see, the first option could work, I will try implement it and make another commit.

[brandonroberts]

@itayod is this ready? Will you rebase on master?

[itayod]

almost, just wanted to make few code improvements that @timdeschryver advised me and I think we are good to go :)

[ngrxbot]

Preview docs changes for a6a5629 at https://previews.ngrx.io/pr1445-a6a5629/

[itayod]

@brandonroberts @timdeschryver have you seen my update? think we can close that now?

[timdeschryver]

I think the implementation is good, the comments are mostly for the created tests.

Wait until @brandonroberts approves the implementation, otherwise, chances are you would have changed these tests for nothing.

[itayod]

@brandonroberts @timdeschryver have you seen my last commit?

[ngrxbot]

Preview docs changes for f7cd4fb at https://previews.ngrx.io/pr1445-f7cd4fb/

[ngrxbot]

Preview docs changes for 3f078cb at https://previews.ngrx.io/pr1445-3f078cb/

[brandonroberts]

Couple more changes and we should be good to go.

[itayod]

Couple more changes and we should be good to go.

done :)

[ngrxbot]

Preview docs changes for 32fc4a6 at https://previews.ngrx.io/pr1445-32fc4a6/

[brandonroberts]

@timdeschryver you have anymore feedback on this?

[timdeschryver]

LGTM, just some few nits.

Also, while testing this I encountered the following:

• The user has to define the reducerFactory if she uses an injection token. We can leave it as is now, but I think 99% of the cases are using the default combineReducers. Should we fall back to combineReducers, if the user doesn't specify the reducerFactory (just like in forFeature)? We can leave it for now and see how it goes.

• If we provide a meta-reducer and an initial state (via forFeature, or via an injection token) - the initial state gets "lost". I think this is a bug.

[brandonroberts]

The reducerFactory should default to combineReducers if not provided when using an injection token. The initialState issue is a bug that's already been filed I believe.

[timdeschryver]

👍
The problem is the the metaReducer is called first without an initial state, resulting in the "default initial state". Thus the provided initial state is ignored.

I'll take a look at the issues, if it isn't filed - I will create a new issue.
But that's something for next year.

In advance, a happy new year 🎉 !

[itayod]

happy new year!!

I will make another commit with the required changes right away :)

about the initial state bug... if you would like, I am available to try solving it and make another pr.

[ngrxbot]

Preview docs changes for 84cf8f5 at https://previews.ngrx.io/pr1445-84cf8f5/

[ngrxbot]

Preview docs changes for 37e4f69 at https://previews.ngrx.io/pr1445-37e4f69/

[timdeschryver]

LGTM

[ngrxbot]

Preview docs changes for 6c801b1 at https://previews.ngrx.io/pr1445-6c801b1/

[brandonroberts]

@itayod keep the fix for the initial state in a separate PR so we can merge this one on its own.

[itayod]

@brandonroberts
ok, but I thought about using the metaReducer suite to test this bug fix...
should I wait for this pr to be merged? or should I create a separate suite?

[brandonroberts]

🎉