cassiozen / useStateMachine

The <1 kb state machine hook for React
MIT License
2.38k stars 47 forks source link

Access context inside effect #33

Closed arthurdenner closed 3 years ago

arthurdenner commented 3 years ago

Hey! πŸ‘‹πŸ½

I'm trying out this library and I'm wondering about accessing the context within an effect.

My question regards something like this:

  1. Effect on initial state updates context and transits to another state
  2. Effect on the next state uses data set by the previous effect <- this is the part I'm asking about
Example ```js import useStateMachine from '@cassiozen/usestatemachine'; const [state, send] = useStateMachine({ count: 0 })({ initial: 'first', states: { first: { on: { second: 'second' }, effect(send, update) { update(context => ({ count: context.count + 1 })); send('second'); }, }, second: { effect(send, update) { // <- context isn't accessible here console.log(state.context.count); // is this the only/correct way to access it? }, }, }, }); ```

Being recently new to state machines, I'm wondering if this is by design and/or if context could be a parameter to the effect function. If could be added, I'd love to contribute to it!

I have used xstate a bit and there I can access context within actions and services. useEffectReducer allows access to context within effects.

cassiozen commented 3 years ago

Thanks for opening this issue. It does make sense, let's implement it.

arthurdenner commented 3 years ago

Cool! Should it be the last parameter in the effect or the first (this would be a breaking change)?

I'll start as the last parameter and we can change it if you see fit.

cassiozen commented 3 years ago

Thanks a lot for the PR!

I will not merge it immediately since there are a few considerations:

RichieAHB commented 3 years ago

Just referencing this comment in what seems to be the appropriate place - https://github.com/cassiozen/useStateMachine/issues/35#issuecomment-850913336

IIUC this comment suggests the idea of not adding a means of accessing context in an effect, in favour of allowing passing arbitrary data with an event (presumably this means context can be accessed by passing it along with the event). The issue I see here is that, because context is never accessible from within an effect, it always requires that any piece of context is passed β€œback in” from an external event. It sort of feels like asking a caller of a method to pass in some value of a public field. Slightly unnecessary and provides room for inconsistency. That said, I may have completely misunderstood this!

My other question is (and this is probably my ignorance), why is a guard better than a fat effect? And if there is a benefit, then people should use guards despite the fact they could be implemented within an effect.

cassiozen commented 3 years ago

There's no wrong answer here. In my opinion, splitting your code between effects (things that happen as a side-effect of a state successfully changing) and guards (conditions that must be matched for a state to be allowed to change) makes things more readable.

I'll close this for now since we're going to ship #37 with events payload. If there is a practical reason to reopen this, we can investigate again for a future release.

cassiozen commented 3 years ago

Reopening, tentative for 0.5.0

arthurdenner commented 3 years ago

@cassiozen, sorry for the delayed feedback - I usually look at open-source notifications on weekends.

I've read all of the discussions and I like the new API with update(callback).send()!

I still think we should allow an effect to read the context, so thanks for reopening this issue.

You pointed out the context being stale inside an effect. I think we can compare it with using the useState hook - we can't call setState and access the updated state in the next line.

cassiozen commented 3 years ago

The better comparison would be with useEffect: You can get in a state prop inside useEffect, but useEffect allows you to pass an array of dependencies to fix that. There's no dependency array for the useStateMachine's effects, so we have to provide some other sort of solution so that users can get the latest context.

cassiozen commented 3 years ago

consideration: Adding context as a param to effect means effects will now expect 4 arguments - might be annoying to use if you will only use a couple (like update and context). Might be the appropriate moment to introduce a breaking change and pass a single object instead.

In E.g.: Instead of this:

effect(_, update, _, context) {
    // ...
}

this:

effect({update, context}) {
    // ...
}

Other considerations:

arthurdenner commented 3 years ago

Great considerations, I support the change of signature.

Regarding guards, do you mean just passing an object or how it works in general? I have some questions/considerations on how guard works but I opened #46 for that.

cassiozen commented 3 years ago

@arthurdenner - Accessing context inside effects is now available on @cassiozen/usestatemachine@1.0.0-beta.1 If you have time, can you take a look and share your impressions?

gchallen commented 3 years ago

I'm seeing some confusing behavior that I think is related to this change. Specifically, if I return a completion method from an effect that uses setContext, any changes made are not visible to the entry function for the next state:

// Trimmed down example that may or may not actually work, but hopefully makes the point
const [state, send] = useStateMachine<{ test?: boolean }>()({
    initial: "recording",
    states: {
      recording: {
        on: { STOP: "replaying" },
        effect({ context, setContext }) {
          return () => {
            setContext(() => ({ test: true }))
          }
        },
      },
      replaying: {
        on: { RECORD: "recording" },
        effect({ context }) {
          console.log(context?.test!) // prints undefined
        },
      },
    },
  })

I'd expect the finalizer for the recording state to run before the entry method for the replaying state, and any changes that it makes to the context object to be visible.

I can probably refactor my code to work around this, but it may be worth a note in the documentation. In my case the reason to do it this way is that the recording method returns a stop handle which has to be used to stop the recording, and it's easier to do that within the same method rather than storing that handle somewhere so it's visible in the entry method for the replaying state.

cassiozen commented 3 years ago

This was a good catch, @gchallen - and it seems to be related to the way React works. Since this is built on top of useReducer and useEffect, when a transition happens, it will forcefully run the exit effect followed by the entry effect. If the exit callback does anything that causes a state change, it will be scheduled to happen after the entry callback.

I will investigate any possible solutions for this, but it might not be simple.