RFC - New Action Handler binding API within a state

[markwhitfeld]

Overview

We want to discuss the options for an alternative action handler syntax within a state.

These are the guiding design goals that we would want to achieve:

1. The StateContext type should be inferred from the state class.
   We don't need to repeat the type over and over, as is the case currently.
2. The Action payload type should be inferred from single declaration of the type.
   Currently, the Action type is declared twice, once in the @Action decorator and again in the function argument type.

Design Goal 1

This can be achieved if we have a base class for a state that exposes the functions of the context (getState, setState, patchState, dispatch) as protected members of the base class with the correct inferred model type.

Design Goal 2

This is what is up for discussion in this RFC...
We would like a clean API for binding a function that handles an action (in the context of a state) with the type of the action it is bound to.

There are two options that we have looked at:

Option A

We define a class method as the action handler and a default argument binding that binds the action type:

// (within the state class)
addTodo(action = on([AddTodo]) ) {
  const todos = this.getState().todos;
  this.patchState({ todos: [...todos, action.newTodo] });    
}

// example with cancellation
loadTodos(action = on([LoadTodos]) ) {
  const todos = await this.todosApi.search(action.filter);
  this.patchState({ todos }});
}

Option B

We have an onAction function that binds the action type to a handler function:
(This value is saved to a protected field)

// (within the state class)
addTodo = onAction(
  [AddTodo], 
  (action) => {
    const todos = this.getState().todos;
    this.patchState({ todos: [...todos, action.newTodo] });    
  }
)

// example with cancellation
loadTodos = onAction(
  [LoadTodos], 
  async (action) => {
    const todos = await this.todosApi.search(action.filter);
    this.patchState({ todos }});
  }
)

Please submit your thoughts about these two options.
Mention which one is your favorite, but please also state what you do and don't like about both options.

If you have another idea, please go ahead and propose it here too!

[arturovt]

My first note on the onAction.

With the following implementation:

class MyLocalState {
  addTodo = onAction([AddTodo], action => {
    const todos = this.getState().todos;
    this.patchState({ todos: [...todos, action.newTodo] });
  });
}

The action handler implementation is not part of the local state class itself; it’s an anonymous function created each time MyLocalState is initialized because it’s essentially part of an instance property. Its this is always bound to the MyLocalState instance scope.

I’m not sure how well instance properties will work when we start dealing with class extensions. Consider a scenario where we have a base local state class that defines properties and is then extended by child classes:

function onAction(actions: any, actionHandler: any) {
  // ...
}

class AddTodo {}

class BaseState {
  addTodo = onAction([AddTodo], action => {
    // ...
  });
}

class ChildState extends BaseState {
  addTodo = onAction([AddTodo], action => {
    // Error: Class field 'addTodo' defined by the parent class is not
    // accessible in the child class via super.
    super.addTodo(action);
  });
}

[profanis]

I would prefer the onAcation between these two options since this API looks similar to the createSelector. This doesn't mean that I do not like the option A.

I liked @arturovt point of view. However, I am not sure how often we would need to extend an Action Class since they need to be unique to their context.

[joaqcid]

i prefer Option B, syntax feels more ngxs and is more readable to me

[markwhitfeld]

After some extensive discussions around this with the RFCs Working Group, we came to the following conclusion:
Option B in the proposal is:

• ✅ a slightly more self-explanatory and explicit syntax (which seems preferable to most developers in the working group)
• ✅in the same sort of style as the new input and output signal component bindings
• ✅feels similar to the syntax of createSelector
• 🫤is only available for binding during the construction of the state because it is a field, as opposed to being on the prototype of the class
• ⛔the handler is an arrow function, and as a result has no control over the this that it is operating on (see more details in option A below)

Option A in the proposal is:

• ✅has the least boilerplate of all the options
• 🫤a little more "magical" in terms of how it works (it is an unfamiliar mechanism to use parameter defaults in this way)
• ✅is less likely to be used incorrectly (outside of the state)
• ✅is available for binding when the state class is provided, even before it is constructed. This is because the handler functions are available on the state class' prototype.
• ✅the handlers are class functions and not arrow functions, so we have more control over the this reference passed to the handler function. For example, for correct handling of cancellation when promises are used, the state modifying functions need to become no-ops and we need to provide an overridable and isolated instance as this. This is a critical implementation aspect for action handlers and cancellation. This really forces us to abandon option B.

So, due to the limitations in Option B, Option A seems to be the right choice here.