kenkunz / svelte-fsm

Tiny, expressive finite state machines for svelte
MIT License
275 stars 9 forks source link

Typescript error on event invocation with argument #4

Closed f-elix closed 2 years ago

f-elix commented 2 years ago

After the last update with the improved types, I now get an error whenever I invoke an event with an argument.

state.paginate(page);
Error: Expected 0 arguments, but got 1. (ts)
    state.paginate(page);

Thanks a lot for this library, really useful!

f-elix commented 2 years ago

Ok I've looked at your tests and I understand why this error exists. I have to specify the arguments for a given transition on the machine.

How should I proceed if I only want to forward that argument to the _enter action of the next state? If I just specify it on the transition without using it right there, I get an 'unused argument' error.

For example:

const state = fsm('idle', {
  idle: {
    paginate(page: number) {
      // This gives me and 'unused argument' error.
      return 'loading';
    }
  },
  loading: {
    _enter({ args }) {
      const page = args[0];
      // ... actually do something with the argument
    }
  }
});

state.paginate(3);

It's not preventing me from anything, I'd just like not to see any errors/warning/hints because of this :)

kenkunz commented 2 years ago

@f-elix glad to hear you're finding svelte-fsm useful! Thanks for reporting this issue. Based on your example, I believe this is a bug in our TypeScript defs – my fault for not considering this scenario.

@ivanhofer do you have any thoughts on how this could be addressed?

Specifically – this is a valid use case where someone wants to pass args in an event invocation, which are not used in any of the normal action methods, but are used in a lifecycle action (where the the args are forwarded via the args param).

It seems like it would be very difficult to infer the proper event arity (and arg types) in this situation. We would need to detect what possible transitions could occur for a given event, and determine what args indices are used in associated lifecycle events. (And if args is ever operated on as a collection, this just seems impossible.)

Assuming this is not possible (or not unreasonably complex), I would suggest we remove the event argument arity / type checks on event invocations, or perhaps accept any arity and types if the args param is used in any lifecycle events. Thoughts / other suggestions?

ivanhofer commented 2 years ago

Hi @kenkunz I didn't know this is a valid usecase 😅 This one looks a bit tricky, I can try if I can get it to work. I just played around a bit and it seems using both a well defined type and a all-args-any type for a function is not valid. I will try to find a solution it in the next couple of days.

@f-elix menawhile you could prefix the page attribute with a underscrore paginate(_page: number) {. This should remove the error (this coud depend on your configuration).

ivanhofer commented 2 years ago

@kenkunz I have opened a PR that fixes this specific problem: #5 But it is not perfect and will not solve all problems

f-elix commented 2 years ago

Wow that was fast! Thanks a lot.

@ivanhofer I tried that already but my configuration still won't allow it (and I can't really change it). In any case, this is easy to work around, I just thought it should be addressed since it's a core part of the API.

kenkunz commented 2 years ago

Hi @kenkunz I didn't know this is a valid usecase 😅

Agreed… definitely on me for not identifying this scenario.

This one looks a bit tricky, I can try if I can get it to work. I just played around a bit and it seems using both a well defined type and a all-args-any type for a function is not valid. I will try to find a solution it in the next couple of days.

👍 thanks for taking a look so quickly! I'll look at your PR and provide additional comments/feedback there.

kenkunz commented 2 years ago

@f-elix thanks again for submitting this! Just published 1.1.2 to npm which should address this – let me know if you notice any other issues.

Thanks again @ivanhofer for contributing your TypeScript expertise, and for helping address this quickly!

f-elix commented 2 years ago

@kenkunz @ivanhofer Works great! Thanks so much for the quick work, really appreciate it.

Just so you know, my agency switched to Sveltekit and we're going to be using svelte-fsm in production for some of our components!