pelotom / use-methods

A simpler way to useReducers
MIT License
705 stars 22 forks source link

Returning state object from reducer methods loses references to state type #18

Open clarkdave opened 5 years ago

clarkdave commented 5 years ago

Using this code sample:

export const App = () => {
  const [] = useMethods(
    state => ({
      set(value: number) {
        return {
          ...state,
          count: value
        };
      }
    }),
    initialState
  );
};

interface State {
  count: number;
}

const initialState: State = {
  count: 0
};

In (for example) vscode, if you rename the count property within the State interface, it will not propagate the count property in the object being returned from the increment method. Similar operations such as "go to definition" and "find references" will also not work.

Although the typings do require that the increment method returns something that matches the State interface structurally, it has lost the reference to the actual interface.

Aside from being a bit inconvenient, this is a potential hazard because TS allows extra properties in the returned object. Therefore, if one was to rename the count property on the interface the reducer would continue to compile without errors, and would fail in an unspecified manner at runtime instead.

Workarounds for this include:

pelotom commented 5 years ago

In (for example) vscode, if you rename the count property within the State interface, it will not propagate the count property in the object being returned from the increment method. Similar operations such as "go to definition" and "find references" will also not work.

Although the typings do require that the increment method returns something that matches the State interface structurally, it has lost the reference to the actual interface.

Any suggestions on making the typings more refactor-friendly?

Aside from being a bit inconvenient, this is a potential hazard because TS allows extra properties in the returned object. Therefore, if one was to rename the count property on the interface the reducer would continue to compile without errors, and would fail in an unspecified manner at runtime instead.

Workarounds for this include:

  • only use immer to manipulate the state (i.e. always return void)
  • type the reducer method return types explicitly: set(value: number): State

Yeah, it's almost always preferable to "mutate" the state via immer rather than returning a new one, but for some specific situations it's highly useful to be able to do so; the most common being returning the initialState in a reset method. A good rule of thumb is to avoid ever returning a spread with some properties modified, as in your example; almost always in that case you should be mutating the state directly instead.

clarkdave commented 5 years ago

I played around with it for a while but I couldn't figure out a way to do it without explicit type hints. It appears unavoidable that the methods prop has to be generic, and doing so seems to make the type system "lose" the reference. I think this is a relevant issue indicating it's a design limitation: https://github.com/microsoft/TypeScript/issues/23955

FYI this came up because we really liked the pattern and decided to try it with Redux actions and reducers (with which we're not currently using immer).

For now I'm adding explicit return types to each method, and will probably enforce this with a linter.

state => ({
  set(value: number): typeof state {
    return {
      ...state,
      count: value
    };
  }
})

Reassigning state also works to avoid the type hint, but it looks weird:

set(value: number) {
  return (state = {
    ...state,
    count: value,
  })
}