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

Meta reducers block initial state #247

Closed
rjokelai opened this issue Aug 8, 2017 · 10 comments
Closed

Meta reducers block initial state #247

rjokelai opened this issue Aug 8, 2017 · 10 comments

Comments

@rjokelai
Copy link
Contributor

rjokelai commented Aug 8, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request

What is the current behavior?

If adding a single meta reducer, the initial state from the config fails to populate.

Expected behavior:

The initial state should be set in both cases.

Minimal reproduction of the problem with instructions:

Add the following test to modules.spec.ts. When adding the simplest no-op meta reducer, the initial state fails to populate.

  describe(`: With initial state`, () => {
    const initialState: RootState = { fruit: 'banana' };
    const reducerMap: ActionReducerMap<RootState> = {  fruit: rootFruitReducer };
    const noopMetaReducer = (r: any) => r;

    const testWithMetaReducers = (metaReducers: any[]) => () => {
      beforeEach(() => {
        TestBed.configureTestingModule({
          imports: [StoreModule.forRoot(reducerMap, { initialState, metaReducers })],
        });
        store = TestBed.get(Store);
      });
      it('should have initial state', () => {
        store.take(1).subscribe((s: any) => {
          expect(s).toEqual(initialState);
        });
      });
    };

    describe('should add initial state with no meta reducers', testWithMetaReducers([]));
    describe('should add initial state with a simple no-op meta reducer', testWithMetaReducers([noopMetaReducer]));
  });

Version of affected browser(s),operating system(s), npm, node and ngrx:

@rjokelai
Copy link
Contributor Author

rjokelai commented Aug 8, 2017

Seems that the problem is in the compose utility function that only passes on the first argument:

export function compose(...functions: any[]) {
  return function(arg: any) {
    if (functions.length === 0) {
      return arg;
    }

    const last = functions[functions.length - 1];
    const rest = functions.slice(0, -1);

    return rest.reduceRight((composed, fn) => fn(composed), last(arg));
  };
}

By changing it to the following, the test starts to pass

export function compose(...functions: any[]) {
  return function(...arg: any[]) {
    if (functions.length === 0) {
      return arg;
    }

    const last = functions[functions.length - 1];
    const rest = functions.slice(0, -1);

    return rest.reduceRight((composed, fn) => fn(composed), last(...arg));
  };
}

rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 8, 2017
rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 8, 2017
@rjokelai
Copy link
Contributor Author

rjokelai commented Aug 8, 2017

Looking better into it, it's probably the usage of the compose method in createReducerFactory that should be fixed. I'll make a PR

rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 8, 2017
rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 8, 2017
rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 8, 2017
rjokelai pushed a commit to rjokelai/platform that referenced this issue Aug 9, 2017
@rjokelai
Copy link
Contributor Author

rjokelai commented Aug 9, 2017

I added a couple of more tests to investigate the functionality. Seems that in the previous versions of @ngrx/store, we created the root reducers ourself like this:

const rootReducer = compose(
  metaReducer1,
  metaReducer2,
  combineReducers
)(reducerMap)

There was no problem in using the compose function then as the combineReducers only accepted one argument. Now, in the current version the combineReducers accepts two, the latter of which is the initialState, which incidentally gets dropped by the compose function. It has been working for cases without meta reducers since createReducerFactory uses the compose method only if metaReducers.length > 0 (see here).

This will start working if we add the initially suggested ...args: any[] to the compose method. The downside is that we lose the type safety we get from the compose method providing only one typed argument for the final function. Other way (I'll do a PR of this), is to change how the compose method is used and do a double type assertion of the returned function.

@RaphaelJenni
Copy link

I have the same issue. But it isn't fixed for the current version (4.0.3)
In my project, I have an initial state and a metaReducer. If I want to import them in the forRoot function in the app module it doesn't work. But if I remove the meta reducer it works again. The strange thing is, that the config is designed to accept a metaReducer and an initialState, but doesn't.

My Code:

@NgModule({
  declarations: [
    AppComponent,
    MainComponent
  ],
  bootstrap: [AppComponent],
  providers: [
    Dialogs,
    {provide: NgModuleFactoryLoader, useClass: NSModuleFactoryLoader}
  ],
  imports: [
    NativeScriptModule,
    NativeScriptFormsModule,
    ReactiveFormsModule,
    StoreModule.forRoot(reducers, {initialState: initialState, metaReducers: metaReducers}),
    EffectsModule.forRoot([StoreEffects]),
    CoreModule.forRoot(),
    SharedModule,
    AppRoutingModule
  ],
  schemas: [
    NO_ERRORS_SCHEMA
  ]
})
export class AppModule {
}
export function logger(reducer: ActionReducer<State>): ActionReducer<State> {
  return function(state: State, action: any): State {
    console.log('state', JSON.stringify(state));
    console.log('action', JSON.stringify(action));

    return reducer(state, action);
  };
}

export const metaReducers: MetaReducer<State>[] = [logger];

export const initialState = function () {
  const secureStorage = new SecureStorage();
  return JSON.parse(secureStorage.getSync({ key: StoreEffects.secureStorageStoreKey }));
};

@jpduckwo
Copy link

Can this be reopened? Still an issue I think. Cheers, Joel

@Matmo10
Copy link

Matmo10 commented Oct 13, 2017

Yeah, I'm using ngrx-store-localstorage by @btroncone . It looks here like this metareducer is designed to accept the initial state and override it as necessary...but for some reason the initial state isn't passed into the metareducer whenever I debug.

@Matmo10
Copy link

Matmo10 commented Oct 13, 2017

Actually I just noticed that this was closed with a fix that went into v4.0.5, but v.4.0.3 is the highest version available on npm?

@brandonroberts
Copy link
Member

@Matmo10 4.0.3 is the latest version of @ngrx/store on npm. Will you open a new issue with a reproduction against the latest version?

@Matmo10
Copy link

Matmo10 commented Oct 13, 2017

@brandonroberts Sure - opened #477

@jpduckwo
Copy link

Just FYI for anyone who's interested. I work around this by setting the initial state in the reducer. For me it actually makes more sense like this anyway.

export const initialState: State = {
  loading: false,
  something: new Map,
};

export function reducer(state = initialState, action: something.Actions): State {
  switch (action.type) {
    case something.LOAD:
      return {
        ...state,
        loading: true
      };
...
    default: {
      return state;
    }
  }
}

bhalash added a commit to D4H/ion-ngx-storage that referenced this issue Oct 7, 2019
See: https://ngrx.io/guide/store/recipes/injecting#injecting-feature-config
See: https://github.com/ngrx/platform/blob/60633b770e27f6d984abc91a802df5ea9902d80a/modules/store/src/store_module.ts#L89-L93
See: ngrx/platform#247

Injecting a meta reducer factory only works for the root state, per
issue 247 above. Injecting the entire configuration through a factory is
the correct approach for a feature state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants