feat(store): implements StateToken<T>

[splincode]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngxs/store/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have 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?

Issue Number: #1391

What is the new behavior?

Enhanced Type Safety State Token

// success compile
const TODOS_TOKEN: StateToken<TodoStateModel> = StateToken.create('todos'); 

// failed compile
const TODOS_TOKEN = StateToken.create('todos'); 

// success compile
const TODOS_TOKEN = StateToken.create<TodoStateModel>('todos'); 

Enhanced Type Safety State Decorator

// success compile
const TODOS_TOKEN: StateToken<TodoStateModel> = StateToken.create('todos'); 

@State({
 name: TODOS_TOKEN,
 default: {} // failed compile 
})
class TodosState {
}

@State({
 name: TODOS_TOKEN,
 default: [] // failed compile 
})
class TodosState {
}

@State({
 name: TODOS_TOKEN,
 default: null // failed compile 
})
class TodosState {
}

@State({
 name: TODOS_TOKEN,
 default: {
    todo: [],
    pizza: { model: undefined }
  } // success compile
})
class TodosState {
}

Enhanced Type Safety Selector Decorator

// success compile
const TODOS_TOKEN: StateToken<TodoStateModel> = StateToken.create('todos'); 

@State({
 name: TODOS_TOKEN,
 default: {} // failed compile 
})
class TodosState {
  @Selector([TODOS_TOKEN]) // failed compile
  public static invalid(state: number): number {
     return state;
  }

  @Selector([TODOS_TOKEN]) // success compile
  public static valid(state: TodoStateModel): number {
     return state;
  }
}

Enhanced Type Safety Select Decorator

// success compile
const TODOS_TOKEN: StateToken<TodoStateModel> = StateToken.create('todos'); 

@State({
 name: TODOS_TOKEN,
 default: {} // failed compile 
})
class TodosState {
}

@Component({ .. })
class AppComponent {
  @Select(TODOS_TOKEN) public stateInvalidX$: string; // failed compile 
  @Select(TODOS_TOKEN) public stateInvalidY$: Observable<string>; // failed compile 
  @Select(TODOS_TOKEN) public state$: Observable<TodoStateModel>; // success compile 
}

Easy extensibility, scaling due to easy adapter

StateToken.inject(MyState); // Extensible API
StateToken.inject('myState'); // Extensible API

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@markwhitfeld @arturovt Unfortunately, I can’t implement Angular 5 support + TS 2.7
With only minimal support TS 2.8

[arturovt]

(edited)

[splincode]

https://gist.github.com/LayZeeDK/c822cc812f75bb07b7c55d07ba2719b3

[kuncevic]

That is great, how soon can we see that released?

[markwhitfeld]

Really well done with this PR. That TS typing code looks crazy.
I am so excited about the type safety that this will bring.

I still believe that we should not have this service locator cache for state tokens.
I will have a look at this branch and try to see if I can remove that part and still have everything working.

[splincode]

@markwhitfeld I added tests, the main thing is do not delete them, and help if you can

[markwhitfeld]

I can't figure out why this is failing in CI. It is all happy on my machine.
@splincode could you try this on your machine?

[markwhitfeld]

Ok, I think I fixed it. I had to expose a fake method that returned the type T so that TS did not ignore the generic parameter.

[markwhitfeld]

Looking good so far.
We still need the following:

• Add to docs
• Decide if the name is to stay StateToken or change to StateKey. See 🚀[PROPOSAL]: State Class Token #1391 for discussion.
• Does this still allow for not exposing the model type? See below.

From the proposal #1391:

export const TODO_STATE_TOKEN = new StateToken<TodoStateModel>('todos');
// or if we do not want to expose type information
export const TODO_STATE_TOKEN = new StateToken('todos');

I know that we have a create function here instead of using the new.
Let's say that the user wants to provide the token but does not want to expose the internals of the Model type. Can we do that with the current implementation? Would you just specify any or unknown?

Also, what are the advantages of the create function over the new?

[splincode]

Does this still allow for not exposing the model type? See below.

It makes no sense not to show the type of model, since this will not be useful, but only add an extra entity

What are the advantages over

const TODOS_TOKEN = StateToken.create<any>('todos');  // any model

@State({
 name: TODOS_TOKEN,
 default: {} 
})
class TodoState {}

const TODOS_TOKEN = 'todos';  // string :)

@State({
 name: TODOS_TOKEN,
 default: {} 
})
class TodoState {}

Let's say that the user wants to provide the token but does not want to expose the internals of the Model type. Can we do that with the current implementation? Would you just specify any or unknown?

You can

StateToken.create<unknown>('myToken');

but use any, unknown for create token do not make sense, since we want to understand the state model at the compilation time

Also, what are the advantages of the create function over the new?

Single entry point to create a token. We can manually control the creation constructor. Since the constructor is private, we can change the signature without critical changes next releases.

[markwhitfeld]

It makes no sense not to show the type of model, since this will not be useful, but only add an extra entity

In some cases users would want to treat the data structure internal to their states as private and not expose it. This is along the idea of encapsulating private information. This would force the only access to be through the selectors. This is a reasonable practice.
The token would remain useful in this scenario as a reference to the state for the storage, reset, or similar plugins.

StateToken.create('myToken');

Currently, this throws an error given the current implementation (through the RequireGeneric construct on this line: https://github.com/ngxs/store/pull/1436/files#diff-f9cde3570946a613f1b14bab13df284eR5). I believe that forcing the explicit generic is a good thing but my question is if passing or would satisfy this construct?

I agree with your reasoning for the create function.

[splincode]

The token would remain useful in this scenario as a reference to the state for the storage, reset, or similar plugins.

You can always use

StateToken.create<any>('token'); 

But I'm sure it is not necessary in 90 percent of applications.

StateToken.create('myToken');
Currently this throws an error given the current implementation

Yes, it was specifically designed to motivate the user to specify the data type

StateToken.create<any>('myToken'); // or StateToken.create<MyModel>('myToken');

It is necessary to indicate in the documentation that users try to indicate the data type, so that they understand that this improves typing

[splincode]

@arturovt please review again

[markwhitfeld]

Great!
Let's just wait for a bit more feedback on the proposal before we merge.

[arturovt]

Sorry for that grammatic requests 😝
I also asked to remove public modifiers because:

we have spoken before that the convention in the project is not to include the public keyword on method declarations.

(c) @markwhitfeld

[arturovt]

There are more comments. Click "show hidden" or something like that... Not sure why github doesn't show all...

[splincode]

@arturovt fixed

[arturovt]

LGTM
Ok, I think we have to hold it a little bit... As Mark said maybe some users may have to leave something in the issue...

[markwhitfeld]

@arturovt Could you confirm that you are happy with the docs?

[markwhitfeld]

@splincode Do you think you could add some details to the release announcement post in this PR?

[splincode]

@markwhitfeld yes, we need

[markwhitfeld]

@splincode I merged the announcement PR so that we can add those details to the article as commit in this PR.

[splincode]

Could you update announcement?