cassiozen / useStateMachine

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

"$$initial" event discussion. #66

Closed cassiozen closed 3 years ago

cassiozen commented 3 years ago

Hey @devanshj, in this example, TypeScript is erroring because of the "$$initial" event:

const [machine, send] = useStateMachine({
  schema: {
    context: t<{time: number}>(),
    events: {
      STOP: t<{resetTime: number}>()
    }
  },
  context: {time: 0},
  initial: 'idle',
  states: {
    idle: {
      on: {
        START: 'running',
      },
      effect({setContext, event}) {
        setContext(() => ({ time: event?.resetTime }));
//                                       ^- Property 'resetTime' does not exist on type '{ type: "$$initial"; }'
      },
    },
    running: {
      on: {
        STOP: 'idle',
      }
    }
  },
});

send({ type: 'STOP', resetTime: 10 });

I think it might be confusing for the user to understand what's going on here. And since so far we're not really using the initial event to trigger anything, I'm thinking of reverting back to start undefined. Any objections?

devanshj commented 3 years ago

I think let's not spoonfeed the users. Let them write if (event.type === "$$initial") return; in the first line they'd get a better understanding on how the machine runs and by making it undefined they'd be more confused why it's undefined in some effects (the ones which would get initial) but defined in others, what's going on they'd ask. And undefined isn't self explanatory, it has to go with an explanation, it's intent is not clear as it's not explicit hence it's a bad idea imo.

cassiozen commented 3 years ago

Take Redux as a comparison example: They do dispatch an initial event, but it's only for internal usage - the user never has to deal with it.

Besides, dealing with an undefined value nowadays is just a matter of using the optional chaining, with makes the user code short and succinct.

I also don't think it's unreasonable for the user to expect that in the initial state the event can be undefined - no events were formally sent yet (from the user's perspective).

If there were any actually internal initialization happening with the $$initial event I would concede, but in this case, we're just changing undefined for $$initial.

I'm sorry I didn't point this earlier and you already implement this, but would you be OK with the decision to revert it?

cassiozen commented 3 years ago

Ah, on a different note: I merged the beta branch into main. I'm using Github's wiki for documentation, and it's not branch-based: Since I'm updating them I might as well make the development happen on main.

In the future I might need a different documentation solution.

devanshj commented 3 years ago

I don't mind per se, you reserve the right to take the final call but let me argue for the sake of argument.

Take Redux as a comparison example: They do dispatch an initial event, but it's only for internal usage - the user never has to deal with it.

I know I used redux as an example previously but it's not a 1:1 comparison, a better comparison would be xstate -- there you do have to handle xstate.init events.

in the initial state the event can be undefined - no events were formally sent yet (from the user's perspective).

This is a good argument.

But what really irks me is that undefined is just not explicit, I mean what if some users go "Eh? Why event is undefined here? It was defined in other effects? Huh must be some bug" *proceeds to write event!.resetTime* and gets a runtime error.

And more importantly let's say you need to do something precisely on initial, take this example

useStateMachine({
  schema: { events: { LOGIN: t<{ user: User }>() } }
  initial: "loggedOut",
  states: {
    loggedOut: {
      on: { LOGIN: "loggedIn" },
      effect: ({ event, send }) => {
        if (event.type === "$$initial") {
          let user = await tryLoggingInWithCookie()
          if (user) send({ type: "LOGIN", user })
        }
      }
    },
    loggedIn: {
      LOGOUT: "loggedOut"
    }
  }
})

This is how this would read with undefined...

useStateMachine({
  schema: { events: { LOGIN: t<{ user: User }>() } }
  initial: "loggedOut",
  states: {
    loggedOut: {
      on: { LOGIN: "loggedIn" },
      effect: ({ event, send }) => {
        if (event === undefined) {
          let user = await tryLoggingInWithCookie()
          if (user) send({ type: "LOGIN", user })
        }
      }
    },
    loggedIn: {
      LOGOUT: "loggedOut"
    }
  }
})

At first look I have no idea what event === undefined even means, it translates to nothing you'd really need to ask a coworker what the heck is this and they'd tell you "Oh that is so that we try logging in only initially and not after the LOGOUT event, you see initially the event is undefined"

Now one'd say "Well you can write if (event.type !== "LOGOUT")" but it still is an indirection and doesn't read as good as if (event.type === "$$intial"). And guess what? if (event.type !== "LOGOUT") is more bad than if (event === undefined) I'll tell you how -- imagine if we have another event that goes to loggedOut state something like this...

useStateMachine({
  schema: { events: { LOGIN: t<{ user: User }>() } }
  initial: "loggedOut",
  states: {
    loggedOut: {
      on: { LOGIN: "loggedIn" },
      effect: ({ event, send }) => {
        if (event.type !== "LOGOUT") {
          let user = await tryLoggingInWithCookie()
          if (user) send({ type: "LOGIN", user })
        }
      }
    },
    loggedIn: {
      LOGOUT: "loggedOut",
      CRASH: "loggedOut"
    }
  }
})

Here the author forgot to add && event.type !== "CRASH" and this will need to update everytime a new event makes the machine transitions to loggedOut so writing if (event === undefined) is the only option and it reads nowhere as good as if (event.type === "$$initial")

Look, I have a philosophy of being "principally correct" or "existentially correct" rather than "utilitarian correct" (in programming and in general). And I'm very intuitive at knowing when something is principally incorrect even if it's utilitarian correct. So I could sense that undefined is principally incorrect and I know if something is principally incorrect there is always a hole and hence I sat here for like 10 minutes to see where the hole is hahaha xD -- these above critics were a result of that. If I think more I can probably come up with more cases like this.

Like I say even in this instance the advantage of being principally correct is that you don't have to think of edge cases, so when I suggested $$initial I didn't even had these cases in my mind.

And please feel free to just go with undefined anyway without arguing or convincing me if you don't want to (it's going to be hard and I'm very argumentative xD) I won't mind at all, as I said you reserve the right to take the call and do you don't owe me an explanation, heck that'd be the truest way to decline :P

cassiozen commented 3 years ago

And please feel free to just go with undefined anyway without arguing or convincing me if you don't want to (it's going to be hard and I'm very argumentative xD)

I very much enjoy our arguing - I'm learning a lot from them.

Let me think a little bit more about $$initial, though.

devanshj commented 3 years ago

I very much enjoy our arguing - I'm learning a lot from them.

Haha I'm glad

Let me think a little bit more about $$initial, though.

Sure, till then I'll keep spamming counter arguments if you don't mind :P Like I realized one now -- in the above code there is an unnecessary rerender because of setContext(() => ({ time: event?.resetTime })) and in my version the user would simply skip by writing if (event.type === "$$initial") return;

And wait, setContext(() => ({ time: event?.resetTime })) won't even compile because event?.resetTime would become undefined | number and time is expects number so the optional chaining is no good and the user would anyway have to write if (!event) return

cassiozen commented 3 years ago

And wait, setContext(() => ({ time: event?.resetTime })) won't even compile because event?.resetTime would become undefined | number and time is expects number so the optional chaining is no good and the user would anyway have to write if (!event) return

Oh, this is reminiscent of the old example. If it were undefined, and I have a desired default value (say, 0), I could use setContext(() => ({ time: event?.resetTime ?? 0 }))

devanshj commented 3 years ago

Oh, this is reminiscent of the old example. If it were undefined, and I have a desired default value (say, 0), I could use setContext(() => ({ time: event?.resetTime ?? 0 }))

Ah that's slightly better but you won't have an default in every case and in some cases the default would the initial context, so the actual way to write this would be setContext(c => ({ time: event?.resetTime ?? c.time })) here c.time just happened to be 0. And ofc that still doesn't discount the fact that it's an extra render.

And I'd go on to say writing it as ?? 0 is much worse because now you'd have to keep the initial context and this in sync,

cassiozen commented 3 years ago

It does make sense. closing, let's keep the initial event.

devanshj commented 3 years ago

And here you're lucking that the compile error reminds you for a default value, that won't be in all cases, consider this...

let email = useEmailFromDatabase() // return string | undefined
let [state, send] = useStateMachine({
  schema: { event: { SET_EMAIL: t<{ value: string | undefined }>() } }
  context: email,
  initial: "editing",
  states: {
    editing: {
      on: {
        SET_EMAIL: "editing",
      },
      effect: ({ event, setContext: setEmail }) => {
        setEmail(() => event?.value)
      }
    },
  }
})

This compiles and has a bug -- can you spot it? Yep it wipes off the initial email that came from database to undefined on initial. Here the context accepts undefined so it compiled but the correct way to write it would be...

effect: ({ event, setContext: setEmail }) => {
  if (event === undefined) return;
  setEmail(() => event?.value)
}

Note setEmail(e => event?.value ?? e) is also incorrect because send({ SET_EMAIL: undefined }) is supposed to set email to undefined.

One has to be careful with optional chaining, with my version there would be no question of this bug because they won't be able to access .value unless they have filtered out the initial event like this...

effect: ({ event, setContext: setEmail }) => {
  if (event.type === "$$initial") return;
  setEmail(() => event.value);
}

If I think more I can probably come up with more cases like this.

As I said :P it took me some 20-25 minutes of thinking to come up with this, because I know for sure there is a hole I just need to find way of exploiting it and come up with horribly modelled code like the above xD

devanshj commented 3 years ago

It does make sense. closing, let's keep the initial event.

Ha nice! A little late, already got another counter xD