RFC: More verbose error message for unprovided feature state in development mode

[jtcrowson]

TL;DR: Add helpful error in development mode when the developer attempts to access feature state that has not been provided.

As a newer NgRx developer, I occasionally had a bug where I was attempting to access state via a selector, but got a console error ERROR TypeError: Cannot read property 'books' of undefined. The issue was, I was using the selector in a module in the app where the StoreModule had not yet been provided with the feature state via StoreModule.forFeature('books', reducers). This module was usually accessed after navigating to another page first, who's module provided the correct feature state, so it was hard to track down.

My proposal is to add a more verbose error message in development mode at the createFeatureSelector level to provide some insight to the developer that they likely did not provide the feature State.

Proposed solution

export function createFeatureSelector<T>(
  featureName: string
): MemoizedSelector<object, T>;
export function createFeatureSelector<T, V>(
  featureName: keyof T
): MemoizedSelector<T, V>;
export function createFeatureSelector(
  featureName: any
): MemoizedSelector<any, any> {
  return createSelector(
    (state: any) => {
      const featureState = state[featureName];
      if (isDevMode() && featureState === undefined) {
        console.error(`The feature name \"${featureName}\" does not exist in the state's root, therefore createFeatureSelector cannot access it.  Be sure it is imported in a loaded module using StoreModule.forRoot(\'${featureName}\', ...) or StoreModule.forFeature(\'${featureName}\', ...).`);
      }
      return featureState;
    },
    (featureState: any) => featureState
  );
}

If accepted, I would be willing to submit a PR for this feature

• Yes (Assistance is provided if you need help submitting a pull request)
• No

[brandonroberts]

This could also be a false positive in the case of using router-store where everything is connected correctly, but the state isn't populated until the first router event happens.

[jtcrowson]

Would the developer get an error about the undefined property in that case?
ERROR TypeError: Cannot read property 'router' of undefined

[evgenyfedorenko]

Maybe there should be more generic message then which covers the router case too ?

[jtcrowson]

Definitely should make the message more generic. I think any error raises suspicion, so even getting a ERROR TypeError: Cannot read property 'router' of undefined because the router state isn't populated is helpful so devs don't think there's an issue. Also, think it makes sense to limit the error to non-production builds only.

[evgenyfedorenko]

So this is a non blocking error? Store can recover from it easily?

[jtcrowson]

Well in the case of router-state, store is fine. But the most common case, the store has not yet been set up but it is trying to be accessed.

[timdeschryver]

I'm not certain but would this give errors because of the isDevMode() mode?

[jtcrowson]

@timdeschryver It should not give errors, because the block of code containing isDevMode() would get executed by the application itself, guaranteeing it is not executed until after the application has been initialized. Tested it to verify as well 👍

[jtcrowson]

@brandonroberts could I get more information on the router-state? I was unable to reproduce. Would the user still get an undefined error? Do we think this message would be worth adding in development?

[timdeschryver]

(sorry to interrupt)
@jtcrowson the router reducer doesn't have an initial state (so it's still undefined) and will create new state on each navigation. https://github.com/ngrx/platform/blob/master/modules/router-store/src/reducer.ts#L21

Thus the selector will first receive the undefined state (because this is before the first navigation), resulting in the message being logged.

[kubk]

Just encountered this error because of a typo. Would be nice to have it fixed.

[jtcrowson]

I'm still in favor of moving forward with this error message in development mode only. I can note that this error is not an issue for router state. Thoughts @brandonroberts @timdeschryver? I think it also helps in case of the router state, since an error is already being logged without explanation.

[brandonroberts]

I think there is less value in this due to #2017, but it could provide some helpfulness.

[jtcrowson]

Agreed, the typo cause should hopefully be less common. For me, this is more about accidentally not registering the state all together but trying to access a selector.

[brandonroberts]

👍🏿

[Halt001]

Is there any way to suppress this warning specific for the router state? Having warnings that one should always ignore is not good practice imho.

[Halt001]

Our unit test output is now littered with lots of this warning complaining about the undefined router in the state. This totally wrecks our unit test output.

[timdeschryver]

@Halt001 could you create a new issue please?

[meanstack-perficient]

actually this error produces the undefined message stated above
yes I am receiving the type error

and takes out the ngrx store itself

so this breaks ngrx feature state (just a spinner) in my case

i remodeled my feature state and selectors... blue in the face

cant shake it

can someone escalate this, reopen or provide workaround

/vendor.js:197218 The feature name "feature" does not exist in the state, therefore createFeatureSelector cannot access it. Be sure it is imported in a loaded module using StoreModule.forRoot('feature', ...) or StoreModule.forFeature('feature', ...). If the default state is intended to be undefined, as is the case with router state, this development-only warning message can be ignored.

ERROR TypeError: Cannot read property 'spinner' of undefined

[jtcrowson]

@meanstack-perficient This error doesn't provide any different behavior for NgRx, all it does is produce a warning if your state hasn't been initialized. Are you sure this warning isn't correctly alerting you to a problem where your state isn't provided?

[meanstack-perficient]

thanks jt

what i have been seeing in the field are angular component developers are creating components that are bloated and case logic is excessive to the point where managing an aggressive angular project becomes unmanagable and unresponsive to velocity.

so I am boilerplating all my talks to service layer in ngrx

which means alot of modules

what I also observed in the field, these angular components talking to the back end with no state mgt or boilerplating is utterly littered with async threads of execution calls, init, etc... causing multithreaded contention especially when trying to introduce more module oriented scaffold

my scaffold houses 4 main large modules core, feature, biz and shared

it sits on ionic which forces the app to slide into tabs.module and lazyload all its dependent tab modules

but the lazy loading is presenting an even more async contention so I am trying to leave out any lazy loading to have something reasonably predictable for getting the app up on its feet

so yes your point is valid, something dependent in my modules isnt being initialized at the proper time and im sifting thru the modules as they pertain to the init load phase

I do have a nice ngrx spinner that starts running on boot so that likes to run in the midst of boot also

but Im not as impressed with the lazy loading and the async contention overall due to the race to initialize everything

so I get that warning

and after that if my ngrx state is not initialize properly I get the error

and I kinda felt maybe ngrx or angular could be more adaptable

but it seems I am caught up in the race condition thing of initialization

Im sifting thru it now

it may not be a bug, I will now shortly when I stabilize my scaffold

[aahmyu]

Can you please provide a way to disable this "feature". The console in the dev tools is full of these warnings and it makes my life harder.

[Klaster1]

@aahmyu https://stackoverflow.com/q/58214913/333777

[dezmon-fernandez]

Agreed please provide a way to suppress these messages, our app relies on dynamic reducers via store.addReducer, since we can have x number of sources at any time. Plus unit test litter is no bueno

[Halt001]

Work is already being done, see here:
#2163