Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic over states and transitions #11

Merged
merged 1 commit into from
May 15, 2021

Conversation

RunDevelopment
Copy link
Contributor

PR for my suggestion under your reddit post.

Type changes:

  • Removed all any types and as casts (except for one). This means that everything is now verified by TS.
  • Removed the KeysOfTransition helper type. TypeScript can infer all state and transition types, so this helper is no longer necessary.
  • Removed BaseStateConfig and BaseConfig. These were purely internal types. I don't know why you added them in the first place. I just used MachineStateConfig and MachineConfig everywhere.
  • The type parameter of useStateMachine is no longer constrained. From what I see, this only got in the way of TS inferring the type. I'll re-add it if it's important.
  • useStateMachineWithContext is now generic over states and transitions and not over the config type. This allows TS to infer all states and transitions. However, this is a breaking change for TS users.

Behavioral changes:

  • reducer: I changed the currentState?.on to currentState.on. Since the state machine is now type-checked, users will get a compilation error if they try to transition into an undefined state. Therefore currentState must be non-nullable.
  • Context will no longer default to {}. This was actually a bug of the old implementation. I fixed this bug here because TS would not compile (that's the advantage of not using any and as :) ). Example:
    const [state, send] = useStateMachine<{ foo: { bar: number } }>()({
        initial: 'inactive',
        states: {
            inactive: {
                on: { TOGGLE: 'active' },
            },
            active: {
                on: {
                    TOGGLE: {
                        target: 'inactive',
                        guard(context) {
                            // no type error but this will crash at runtime
                            return context.foo.bar > 4;
                        }
                    }
                },
            },
        },
    });
    
    console.log(state); // { value: 'inactive', nextEvents: ['TOGGLE'] }
    
    send('TOGGLE');
    
    console.log(state); // { value: 'active', nextEvents: ['TOGGLE'] }
    
    send('TOGGLE'); // crash

Feel free to request changes.

You can try out everything here.

@cassiozen
Copy link
Owner

Wow, @RunDevelopment, this is spectacular; thanks so much for your time!

I don't know why you added them in the first place.

Because I lack knowledge. This is the first project where I use TypeScript beyond the basics...

The type parameter of useStateMachine is no longer constrained. From what I see, this only got in the way of TS inferring the type. I'll re-add it if it's important.

It makes sense now. I don't think we need to re-add.

Therefore currentState must be non-nullable

👍

Context will no longer default to {}

👍

I also noticed you changed the reducer to always accept a type in the action and expose send as a function that dispatches { type: "Transition" ... instead of exposing the useReducer dispatch directly. A clever solution, very obvious, I don't know why I didn't think of that from the beginning 😅.

One change I will do in this case is renaming the internal send to dispatch to be more idiomatic - but I can do that separately, you already contributed with a lot.

Overall, thanks again a lot for your PR - this is as much an improvement for the library as a learning opportunity for me.
Please let me know if you think of any other suggestions in the future.

@cassiozen cassiozen merged commit 29abc11 into cassiozen:main May 15, 2021
@RunDevelopment RunDevelopment deleted the generic branch May 15, 2021 14:36
@cassiozen
Copy link
Owner

Oh, I remembered why I this all that trickery with the send method: I wanted to completely reuse dispatch because it's stable across re-renders. There is the possibility that some user will pass send to child components - if these components use PureComponent or React.memo, an unstable send method will make them useless.

Not a big issue, can be definitely dealt with on a separate occasion.

@RunDevelopment
Copy link
Contributor Author

Ahh, that's true. The methods we give out might behave the same but they aren't the same object.

I wonder, is there an idiomatic way to do this in the React world or will it be a WeakMap solution?

@cassiozen
Copy link
Owner

Yes, I can use either useMemo or useCallback.

@cassiozen
Copy link
Owner

@all-contributors please add @RunDevelopment for code, test and ideas.

@allcontributors
Copy link
Contributor

@cassiozen

I've put up a pull request to add @RunDevelopment! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants