Store: 🚀[PROPOSAL]: State Class Token

[markwhitfeld]

Relevant Package

This feature request is for @ngxs/store and potentially some plugins

Description

While I was looking through @arturovt's PR #1380 on using the State class as a key in the storage plugin it reminded me of an idea I had from a while back.

Sometimes we want to refer to a state and currently have had two mechanisms for doing so: By the actual class or by the name of the class (the path in the state tree).
Both these approaches have their drawbacks and advantages:

• The name approach:
  
  
  
  • ☹️ Lacks model type information
  
  
  • ☹️ Results in the value appearing in the state as well as in other parts of the app (unless shared in a const value)
  
  
  • ☹️ Does not participate well in refactorings
  
  
  • 👍 Is a good option when we want to refer to some state that may not exist yet (part of a lazy module)
  
  
  • 👍 Provides an abstraction from the actual State class
  
  
• The State Class approach:
  
  
  
  • ☹️ Also lacks model type information
  
  
  • ☹️ Requires the State class to have been loaded
  
  
  • 👍 Is safe for refactoring
  
  
  • 👍 Allows for a consolidated reference to a part of the state tree
  
  

Describe the solution you'd like

Maybe we could have all the advantages without the disadvantages...?

If we had a token that represents a state class without being the state class itself then it could be substituted wherever the name or State Class is used as a token to represent the state class.

The idea is similar to the Angular InjectionToken that could optionally carry type information.
So we could have:

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');

❗️ Point of discussion: Would we follow the same capitalisation convention used by Angular?

Then in the state class we could have:

@State<TodoStateModel>({
  name: TODO_STATE_TOKEN,
  defaults: { }
})
export class TodosState {}

❗️ Point of discussion: Would we want to overload the name property or should we have a separate token property? I think I am happy to overload the name property to keep the concept simple (that it can be used wherever the name or class was used).

We could use it in a Selector:

@Selector([TODO_STATE_TOKEN])
static todoCount(state: TodoStateModel) {
    return state.ids.length;
  }

This can also assist in providing type safety with the @Selector decorator! Something we have been longing for!
Plus, this allows for the decoupled definition of selectors that join multiple states (some of which could be lazy loaded)!

So the drawbacks and advantages of the token would be:

• ☹️ Adds some potential complexity
• ☹️ Adds another concept (although it would be familiar to an intermediate Angular dev)
• ☹️ Would require some plugins to be updated to make the concept ubiquitous
• 👍 Optionally can include model type information
• 👍 Results in the state name value appearing only in one place
• 👍 Participates well in refactorings
• 👍 Provides an abstraction from the actual State class
• 👍 Allows for a consolidated reference to a part of the state tree
• 👍 Is a good option when we want to refer to some state that may not exist yet (part of a lazy module)
• 👍 Opens the door for type safety in @Selector decorators

This could be a feature that we aim for in v4 because it would potentially require plugins to add support.
@ngxs/core team, what do you think?

Describe alternatives you've considered

None

[splincode]

You mean?

export class StoreOptions<T> {
 name: string | StateToken<T>;
 ..
}

Or do you want to get rid of the string type completely?

[markwhitfeld]

Yes, it would not drop support for the string.
So, if we overloaded the name property then it would support a string or token.

[markwhitfeld]

@arturovt and @splincode What are your thoughts on the two points of discussion I raised in the description?
@joaqcid you can add your 2c here too if you want...

[joaqcid]

@arturovt and @splincode What are your thoughts on the two points of discussion I raised in the description?
@joaqcid you can add your 2c here too if you want...

i think overloading name is ok, it is clear that name represents the "id" of the state, and using a token makes sense.
one thing that comes to my mind here is, couldn't we use the same approach as in the storage and use the StateClass as the name

about capitalisation, you are asking about the name of the token "TODO_STATE_TOKEN", wouldn't that be defined by each developer?

[markwhitfeld]

@joaqcid Thanks for your thoughts.
I might be missing what you mean here, but using the StateClass as the reference is where we are at currently. It then forces any code that wants to use the state to have a reference to the state class which forces it to be loaded.

Regarding the token, yes the choice of the naming of the state token variable will be up to the developer, but I would like us to decide what we put in the docs to be consistent. What is your preference?

[joaqcid]

I might be missing what you mean here, but using the StateClass as the reference is where we are at currently. It then forces any code that wants to use the state to have a reference to the state class which forces it to be loaded.

what i ask is if we could get rid of name field on the @State decorator options, and use class metadata kind of how it's used in the storage plugin,

(stateClass as any)[OPTIONS_KEY].name

this probably is not possible, but just wondering if there could be a way...

Regarding the token, yes the choice of the naming of the state token variable will be up to the developer, but I would like us to decide what we put in the docs to be consistent. What is your preference?

regarding token name, i like TOKEN_NAME capitalisation

[markwhitfeld]

@arturovt raised the question in a chat about the name StateToken to see if it is the best name and that it conveys the idea sufficiently.
I think that StateToken has a very similar utility to the InjectionToken in Angular so I think that the name could be a good fit... on the other hand, maybe it could get confusing between these and the Angular ones.
An alternative name could be StateRef because it is used as a reference to the state.
What do you think of this name?
Any other name ideas?
@everyone is welcome to add their ideas!

[splincode]

I'm not very sure about the name StateRef. Since this is more like a variable name or some type of data...

I think that StateToken has a very similar utility to the InjectionToken in Angular so I think that the name could be a good fit

I agree

[joaqcid]

by type safety here, does it mean that when we use

@Select([TODO_STATE_TOKEN]) todos;

TS will infer that todos is from TodosStateModel?

Or do we still need to @Select([TODO_STATE_TOKEN]) todos: Observable<TodosStateModel>;?

[arturovt]

@markwhitfeld

I think that StateToken has a very similar utility to the InjectionToken in Angular so I think that the name could be a good fit...

InjectionToken is a provider. StateToken has nothing to do with dependency injection and it shouldn't. It shouldn't become a "God Object". You have mentioned in chat that it must be a typed container.

See StateKey from the platform-browser package. I like its concept, the single type and the single makeStateKey function, nothing complicated.

---

Plain functions are tree-shakable. Classes are not (tree-shakable providers are eliminated on the Angular compiler level). If somebody doesn't use this feature then it shouldn't be included in the production bundle.

[splincode]

@arturovt Then how are you planning fix if we use plain functions
https://github.com/ngxs/store/pull/1436/files#diff-ecebb674f203a03f5d607259e26933edR170
https://github.com/ngxs/store/pull/1436/files#diff-ecebb674f203a03f5d607259e26933edR71

Also for correct work with StateKey we need use TransferState injector

TransferState will be available as an injectable token.

https://angular.io/api/platform-browser/TransferState#description

In our case, we do almost the same thing

@kuncevic What do you think?

[markwhitfeld]

@joaqcid we will still need to @Select([TODO_STATE_TOKEN]) todos: Observable<TodosStateModel>;
The only difference is that we can do type checking for you. Unfortunately, property decorators in TS currently cannot infer the type of the property for you.

[markwhitfeld]

So, @arturovt @splincode @joaqcid would you prefer to call this StateKey?
Or do you want to stick with StateToken?

[splincode]

I want to use StateToken. Because in the future it can really be used for internal state provider

[markwhitfeld]

After looking up the dictionary definitions of token and key I am leaning towards StateToken.

• A key represents "something of critical importance". It has been used in data structures to refer to the piece of information that is critical to its' existence and therefore can be used to look up that piece of data.
• A token serves as "a representation of something", "serves as an object conferring authority in order to gain access", "serves as a voucher that can be used in exchange for goods or services".

I feel like token is a bit closer to what we are talking about.
I think Angular uses it for its' providers because of the alignment with the concepts above. Token does not imply provider. It is merely the thing given to the provider as a representation of what should be provided. In our case the store is our provider of state information and the StateToken represents the part of the data that we want.

@arturovt @joaqcid What do you think?

[splincode]

I agree with @markwhitfeld
The key is still a regular key and carries nothing but this
https://angular.io/api/platform-browser/StateKey

type StateKey<T> = string & {
    __not_a_string: never;
};

All the same, the signature of this type is a string!
But we have an instance at the exit

[joaqcid]

though i feel some arguments from @arturovt are correct, i still like more StateToken for this case.

[splincode]

@joaqcid @arturovt We got rid of some things to improve tree shaking. Mark removed some code, so if you do not use tokens, then the code will not be added to the bundle production.

[markwhitfeld]

I think that it would also possibly be dangerous to use the same name as the existing angular construct called StateKey.

[poloagustin]

Hi!
I really like this idea @markwhitfeld !
+1 to the StateToken naming convention.

I understand that this feature won't add type safety when using the @Selector decorator, but will it make Store.select/Once/Snapshot infer the type when using tokens? If that's true, then I'm sold already :)

I need to let you all know i did not know that referencing the State class in my components were impacting my bundle THAT much! I'm definitely going to think about my bundle more from now on.
So this is a big plus.

[splincode]

I understand that this feature won't add type safety when using the @Selector decorator,

I added type support for decorators so there is type safety there too!

@Selector
https://github.com/ngxs/store/pull/1436/files#diff-96eed1b50d3e46b9cd68f6e9662cb7baR76

@Select
https://github.com/ngxs/store/pull/1436/files#diff-96eed1b50d3e46b9cd68f6e9662cb7baR122

[markwhitfeld]

I need to let you all know i did not know that referencing the State class in my components were impacting my bundle THAT much! I'm definitely going to think about my bundle more from now on.

@poloagustin Maybe I missed something... what are you referring to here?

PS. Thanks for your feedback

[poloagustin]

I'm talking about the fact that doing @Selector(AppState) or store.select(AppState) was putting the entire class in each module i was referencing it.
it might have been my bad, but doing it the token way will be better for reducing bundle size

[splincode]

I'm talking about the fact that doing @Selector(AppState) or store.select(AppState) was putting the entire class in each module i was referencing it.

Is it really? Can you show webpack analysis?

[arturovt]

You can stick with the "token" concept if the community likes it.

My second point was to remove the create method.

In theory it is another level of encapsulation. e.g. if at some point you have some cache of these objects and create won't always construct a new object, but doing this is as a rule is an antipattern.

Since the constructor is private, we can change the signature without critical changes next releases.

Why would you change the signature of the constructor at all?

There is no difference between new StateToken() and StateToken.create. Except the left one is more clear and simpler.

The advantage of factory methods is that they can do something other than construct a new object, e.g. recycle some cached instance; or the fact that you can have multiple named factory methods:

StateToken.createRedStateToken(string)
StateToken.createBlueStateToken(string)

At the end of the day more code is worse. So just more code for the sake of more code :confused:

[splincode]

@markwhitfeld @arturovt

Why would you change the signature of the constructor at all?

As practice shows, this constantly happens.

My second point was to remove the create method.

Factory method needed for future improvements

[arturovt]

Stop talking screenshots, I don't understand what it means.

Factory method needed for future improvements

What exactly?

[arturovt]

By default doing extra work is bad, so unless you can bring up an argument why that extra work SHOULD be done; by default it should not.

"Future improvements" sounds like "let's buy the most powerful machine because we might have 100k+ users in the future".

[splincode]

@arturovt I do not know what you want to achieve, but you should predict the future. One of the main improvements that I want to achieve is that the tokens are unique and do not have duplicates, perhaps in the future, some additional logic will be executed under the hood when creating the token. That's why I was interested in the idea that it should be a singleton. From my point of view, it is easier to maintain without critical changes, since the creation of the token will be performed using a single method.

[arturovt]

you should predict the future

:joy: You don't do a good job of explaining it.

[markwhitfeld]

@arturovt I looked at removing the create method, but it looks like it is necessary in order to enforce the specification of the generic type. I quite like this constraint that forces the user to think about it.
To demonstrate with code, the difference is:

// This will give a type error because you didn't provide the generic type
const myToken = StateToken.create('hello'); 

// But, this will give no error because it infers `unknown` for the generic type
const myToken = new StateToken('hello'); 

This is unfortunate because it forces us to use the create approach.
I had hoped we could avoid the unnecessary create but we have to in this case.

PS. @splincode Did you know that you can still create singletons using the constructor method...

class MyClass {
  private static _singleton: MyClass;
  constructor() {
    return MyClass._singleton || (MyClass._singleton = this);
  }
}

console.log(new MyClass() === new MyClass()); // logs true to the console!

[arturovt]

@markwhitfeld Trust but verify :wink:

I just did the below stuff:

export class StateToken<T = void> {
  constructor(private readonly name: TokenName<T>) {}

And it gave the same output:

new StateToken('name') // Argument of type '"name"' is not assignable to parameter of type '"You must provide a type parameter"'.

Same as the create method does.

[splincode]

class MyClass {
  private static _singleton: MyClass;
  constructor() {
    return MyClass._singleton || (MyClass._singleton = this);
  }
}

console.log(new MyClass() === new MyClass()); // logs true to the console!

After Java, I certainly can’t believe that it’s possible), but alas, JS is such JS. Ahah. It's valid by TS?

[splincode]

@markwhitfeld But invalid for my opinion

console.log(new MyClass('A') === new MyClass('B')); // need false
console.log(new MyClass('A') === new MyClass('A')); // need true

[troydietz]

The token is a neat idea. Anything that improves typing with selectors is a step in the right direction. We've been burned by typing @Select one way in a component, when in fact the @Selector was returning a different type. That being said, I think the way our application is structured, we would see only some of the benefit from this. I believe this is because some of the problems the State Token is solving are not pain points we really feel.

The name approach:
☹️ Lacks model type information
☹️ Results in the value appearing in the state as well as in other parts of the app (unless shared in a const value)
☹️ Does not participate well in refactorings

We really only deal with the name twice. Once in the @State decorator. And once more when bootstrapping the corresponding state's unit test. We aren't traversing the global state object using the name. Are you guys doing this in your application? (question for everyone) . Because of this, I don't find most of the name frowny faces to be problematic.

The State Class approach:
☹️ Also lacks model type information
☹️ Requires the State class to have been loaded

When referring to a StateClass, we are always referring to a static selector defined in the state class file. So I'm not entirely sure why I would want to refer to a State class if the file hasn't been loaded. What are the use cases for this? Full disclosure, I kinda feel like I'm missing something here.

Overall, I think its a cool idea and a step in the right direction. I think the primary benefits I'd see from this would be more type safety in our facade classes. Which is great! 🚀

[markwhitfeld]

@arturovt

@markwhitfeld Trust but verify 😉

Awesome, thanks for verifying!!! I had tried it but missed the T = void!
We can now get rid of the create method. There is no more need for it.

@splincode

But invalid for my opinion

I was just giving an example to remind you that Java is not Javascript 😉

@troydietz
Thanks for your feedback. I agree that it would not be a normal case to use the token. There are some advantages if you need to refer to the state class before it is loaded (for example if you want to tell the storage plugin to persist a part of the state that will be lazy loaded later).

@everyone That being said, I would like to ask if you believe that this should be part of the core framework?

[splincode]

That being said, I would like to ask if you believe that this should be part of the core framework?

Typing will not work without integration with decorators @Select, @Selector, store.selectSnapshot, store.select. Therefore it should be the core @ngxs/store

[markwhitfeld]

That's strange because I made the changes and no tests or typings fail.
I'll push my changes and let's see if the CI is happy.

PS. Please, could we keep the implementation-specific discussion on the PR, because all this back ond forth over the create vs new is adding noise to the proposal.

[splincode]

@markwhitfeld @joaqcid @arturovt @troydietz @poloagustin I want to note that at the moment, we have completed work on this proposal

[markwhitfeld]

If anyone has some further feedback, adjustments or reservations with this, please speak now.
I will hold off merging until this time tomorrow to allow for further feedback.
(Click the 🚀 if you are happy for me to merge!)

[kuncevic]

@markwhitfeld I would like to ask if you believe that this should be part of the core framework?

absolutely!