Fixed #359

[harm-less]

What:

Turned out a different (I think wrong) State object was imported into examples.component.ts. Changing it fixed the build issue 👍

Issue number: #359

[timdeschryver]

More in detail: the typing of Store isn't really useful in our components, it just makes the store typesafe. But since we're using selectors it isn't really adding much to the table, personally I usually declare the state as State<{}> in my components.

[harm-less]

I'm not sure I follow 100% as I'm still trying to get the hang of this repo, but in settings.selectors.ts no such AppState is being used, only just State.

In this scenario I'm assuming the rest of the code is 100% consistent/right, so it could still be you're right, but then my PR changes aren't the problem I guess as the problem runs deeper.

[timdeschryver]

To give you a little overview:

In core.state we have defined our AppState, this module gets loaded in directly when we navigate to any page. Our state is:

{
  auth: {},
}

In settings.model (maybe this has to be renamed to settings.state) we have defined the SettingsState and it is extending the AppState. Just like the core state, this will be loaded in directly.

Our state is:

{
  auth: {},
  settings: {},
}

In examples.state we have defined the ExamplesState and it's also extending the AppState. Now unlike the other states, this one gets lazy loaded once we navigate to the examples page.

Our state is:

{
  auth: {},
  settings: {},
  examples: {
    todos: {},
    stocks: {},
    form: {},
    books: {},
  }
}

In settings.selectors we're grabbing the settings property from our global state with

export const selectSettings = createFeatureSelector<State, SettingsState>(
  'settings'
);

If we take a look back at our state:

{
  auth: {},
  settings: {}, // it takes this property
  examples: {
    todos: {},
    stocks: {},
    form: {},
    books: {},
  }
}

Because of this, if we use selectSettings in any of our components it's actually using the settings property and therefor we could for example do settings.language.

[timdeschryver]

But all of this has nothing to do with how we import the store, we could for example use Store<AppState>, Store<State> or Store<ExamplesState> and so on, and it will still work. This is because the import is just for the typesafety if you would use the following syntax:

constructor(private state: State<AppState>){
   this.store.subscribe(s => s.auth); // s is typesafe and knows it's `AppState`
}

But because we're using selectors it "doesn't care" about the types (our selectors are typesafe tho).

[harm-less]

I understand it better now, but I think the store you're getting doesn't entirely match what you subscribe to in examples.component.ts on line 54:

private subscribeToSettings() {
    this.store
      .pipe(select(selectSettings), takeUntil(this.unsubscribe$))
      .subscribe((settings: SettingsState) =>
        this.translate.use(settings.language)
      );
  }

If you use AppState you don't have the settings store, that doesn't make sense to me. I'm surprised Typescript doesn't complain about that when I tried that. But using State which it is right now, you do have the Settings:

export interface State extends AppState {
  settings: SettingsState;
}

Long story short, I think State should not change to AppState.

I do agree with your comment about renaming State to SettingsState to make it more descriptive. And maybe the current SettingsState can be renamed to SettingsModel or something (that matches the file name and it's descriptive).

[harm-less]

And maybe to clear things up. You said this in your review:

Thanks for filling this PR @harm-less, I think the right State to import should be AppState from @app/core. Because: we're selecting the auth state we're selecting the setting state Could you try this out? (I don't encounter problems so I can't test it)

However, State has settings, AppState does not, so you might have switched things around, right?

[timdeschryver]

Not exactly. You don't have different states, you just have one global state containing every piece of state. That's what I meant with:

{
  auth: {},        // comes from core
  settings: {},    // comes from settings
  examples: {      // comes from eaxmples
    todos: {},
    stocks: {},
    form: {},
    books: {},
  }
}

All of these form your state.

The snippet you posted does the following

this.store
      .pipe(select(selectSettings), takeUntil(this.unsubscribe$))
      .subscribe((settings: SettingsState) =>
        this.translate.use(settings.language)
      );

• call selectSettings with the global state (see json above) from the store
• the selector returns the settings property from the global state
• now when you subscribe settings, this is indeed the SettingsState containing all the settings properties

[harm-less]

All right, thanks for the clarification 😃

Anyway, we still have the problem in the current codebase. I guess both scenarios will work, so how do we resolve this PR?

[timdeschryver]

I think AppState is preferable here, because we're selecting the auth state and the settings state.

[tomastrajan]

@timdeschryver @harm-less hey guys! So it seems to be the case that with "typescript": "^3.1.0" compiler starts to complain about accessing multiple things from the SettingsState state which is typed as ExampleState...

My current solution is interface State extends BaseSettingsState, BaseExamplesState {} but I am open for feedback / improvements. The whole thing then looks like this...

import { State as BaseSettingsState } from '@app/settings';
import { State as BaseExamplesState } from '../examples.state';

interface State extends BaseSettingsState, BaseExamplesState {}
/* ... */

constructor(private store: Store<State>) {}

[tomastrajan]

Please resolve in a way described n the previous comment