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

New warning about unprovided feature state wrecks unit test output #2116

Closed
Halt001 opened this issue Sep 12, 2019 · 14 comments · Fixed by #2163
Closed

New warning about unprovided feature state wrecks unit test output #2116

Halt001 opened this issue Sep 12, 2019 · 14 comments · Fixed by #2163

Comments

@Halt001
Copy link

Halt001 commented Sep 12, 2019

We have unit tests where we use the state without mocking. With the new warning about undefined feature state, that is given while it shouldn't in case of the router state, our unit test are now littered with this warning. Also it shows up in the debugger console when we run the app.

Warning:

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

Also see the discussion here: Issue 1897

image

Expected behavior:

I think it is bad practice to give warnings that should be ignored in some cases. This leads to warning/error fatigue. Also unit tests that have hundreds or even thousands of warnings quickly lose their usefulness.

So please find a way to not warn when there is nothing to warn about, like in the case of the router state. Or add the ability to suppress the warnings for a user provided key, or even remove this warning completely as it does seem to do more harm than good.

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

NgRx 8.3
Ng 8.2.5

@jtcrowson
Copy link
Contributor

jtcrowson commented Sep 17, 2019

@Halt001 Could you push up a basic code sample where this is happening? I can try to work around this, possibly by excluding tests, possibly by excluding router state. Thanks!

@Halt001
Copy link
Author

Halt001 commented Sep 17, 2019

Working on it. May take some time as I can't share our code and it isn't obvious to me when the warning is triggered.

@jtcrowson
Copy link
Contributor

I appreciate your help, thanks!

@Halt001
Copy link
Author

Halt001 commented Sep 18, 2019

Sorry for the delay. I've created a repo with an app that generates the warning multiple times after running the unit tests with npm test.

Also when running the app with npm start you can observe the warning in the browsers debugger console.

@timdeschryver
Copy link
Member

I haven't taken an in-depth but it I believe you should also import the router and router-store. In your component you're using a router selector, but no (router) state is provided.

@Halt001
Copy link
Author

Halt001 commented Sep 19, 2019

I've added actual routing to the app. It now doesn't display the warning in the browser debugger console anymore (but our production app does) but the example still shows the warnings in the unit tests. Hope this may save you some work.

@jtcrowson
Copy link
Contributor

jtcrowson commented Sep 26, 2019

@timdeschryver what do we want to do here? The point of this was to save headaches, not create them. Only approach I've found was to string match on the state key for router. I do think it is a valuable warning, though, as I think the comment in the original thread is an unprovided state issue.

@meanstack-perficient
Copy link

well the error is telling me something is wrong...better than none
Im pushing this framework to the max

for instance if you forget to add an index route ' ' in one of your router modules you will get this

typeerror cannot read property ‘split’ of undefined at defaultUrlMatcher

which doent tell much about what to do or where to go except guess that url/router might be the cause... that one held me up three days deep into router modules

so if this ngrx error is omitted or stifled what would I see then?

@timdeschryver
Copy link
Member

@jtcrowson I have mixed feelings here, the same you are expressing.

I don't see any solution neither, besides doing a string match.

I'd like to know if it's only the router store that's causing these warnings.
We haven't upgraded to 8.3.0 yet at work, but I'll try to push this to next week - just to see the behavior here.

@meanstack-perficient
Copy link

From my perspective

My router store is functioning and running fine
and
this message it has to do with any store in general

and I eluded to race conditions associated with the way developers are cobbling together their lazy loaded and eager loaded modules as they are setting up at bootstrap according to their route tree

I am ironing out my router tree now

in some cases ngrx is ok and gets past the anomalous setup of modules beit eager/lazy
and the message appears

in other cases the message appears identifying the anomalous setup of modules beit eager/lazy

and the reference error occurs

@timdeschryver
Copy link
Member

Thanks for the info @meanstack-perficient

@aquark
Copy link

aquark commented Sep 30, 2019

We're having a similar issue with many user-defined feature modules where the state is intentionally undefined until some event changes the state. We can work around the issue by initializing the state to null instead of undefined, but would rather not. I don't suppose there's a way to use hasOwnProperty to check if a key is present in the store, whether or not its value is undefined?

@timdeschryver
Copy link
Member

I like that idea @aquark 👍!
From what I can think atm, that would fix these problems and it will also provide a helpful message.
Thoughts @jtcrowson ?

@Gunyon
Copy link

Gunyon commented Oct 11, 2019

try to select the router slice of state with a simple function

export const selectRouter = (state: State) => state.router;

because createFeatureSelector, hence the name, is used for selecting a slice of state added
in a FEATURE module

StoreModule.forFeature('feature1', reducers)

in this case you should use createFeatureSelector

export const selectFeature1 = createFeatureSelector<State, Feature1State>('feature1');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants