feat: improve TypeScript definitions

[ivanhofer]

I have added a few TypeDefinitions.
This PR is still WIP.

Implemented typesafety/autocompletion features:

• typesafe initial state
  

• typed subscribe function
  

• typed action calls
  
  
  

• halfly-typed arguments for lifecycle methods
  (the arguments are typed as string, but aren't restricted to the exact state values, like 'initial' | 'final')
  

• only allow actions that the state-machine can handle
  

Please let me know if I made wrong assumptions about the functionality of the state machine code.

[ivanhofer]

@kenkunz do you have the intention to convert all files to TypeScript?
This would add the benefit of type-checking as additional test before you can build & deploy the library

[kenkunz]

@ivanhofer thanks for the PR!

I'm focused on completing a feature and then taking a break for the U.S. Thanksgiving weekend – will take a closer look at this early next week.

Vielen Dank!

[kenkunz]

@ivanhofer I see you've continued to improve the TS declarations 👍

Overall comment: this looks great! I can see how this will improve the dev experience for TS developers and for anyone relying on IntelliSense / IDE feedback. Thanks for your time and effort on this!

I've checked out the PR and started testing the declarations. I will add separate comments to address specific questions or feedback.

[kenkunz]

@ivanhofer replying to your earlier question:

do you have the intention to convert all files to TypeScript?

Not at this time. I don't have much experience with TypeScript – I've dabbled with it just enough to get a feel for it, and find that I prefer vanilla JavaScript. (My favorite language is Ruby – a very dynamic language that intentionally separates the notion of class from type ("duck typing" – if it quacks like a duck, it is one).

This would add the benefit of type-checking as additional test before you can build & deploy the library

I tend to lean heavily on unit tests and dynamic type checking (and only if/where it's really needed). The current surface area of the library is pretty small (and I hope to keep it that way), so I think the risk of using an inappropriate type internally is minimal.

All of that said… I can definitely see the benefits of exposing type declarations for users of the library!

[kenkunz]

1. Basic use case feedback

All the basic cases below make sense… with the exception of the initial state validation (will address in a separate comment).

Let me know if this style of feedback works for you – I'm basically writing test cases with pass/fail (✔︎/✘) comments

const machine = fsm('baz', {                // ✘ leaning against validating initial state
  '*': {
    abc: () => {}
  },

  foo: {
    def: 'bar'
  },

  bar: {}
});

machine.subscribe();                        // ✔︎ warning expected
const unsub = machine.subscribe(() => {});  // ✔︎ no warning expected
unsub.foo();                                // ✔︎ warning expected
unsub('foo');                               // ✔︎ warning expected
unsub();                                    // ✔︎ no warning expected

machine.abc();                              // ✔︎ no warning expected
machine.def();                              // ✔︎ no warning expected
machine.ghi();                              // ✔︎ warning expected (no matching action)

[kenkunz]

2. Initial state in explicitly declared states

Here's an example of why I'm hesitant to validate initial state in explicitly defined states. The code below is something I'm actually using as a "mini in-page router" w/in a SvelteKit app. This wasn't something I initially envisioned doing with svelte-fsm … but making use of the '*' fallback state in this way turned out to be useful for this scenario. Essentially, the mode object can only be in one of the three states defined by the transitions below. Since the transitions are handled by the special fallback state, there's no need to explicitly define each of the states.

const mode = fsm('signIn', {
  '*': {
    signIn: 'signIn',
    signUp: 'signUp',
    resetPassword: 'resetPassword'
  }
});

mode.signUp();        // => 'signUp'
mode.resetPassword(); // => 'resetPassword'
mode.signIn();        // => 'signIn'

More generally, I lean towards keeping it optional whether you explicitly declare all states. A final state doesn't need any transitions or actions. Some people may wish to declare it someFinalState: {} to be explicit. Others may prefer to leave it off for brevity.

One possible way to approach this is to offer a strict option. We could provide a package subpath export for strict.js (with accompanying strict.d.ts declaration), so someone who prefers stricter type validation could import fsm from 'svelte-fsm/strict'; Thoughts?

[kenkunz]

3. State type should be restricted to string | symbol

I noticed you added number as a valid State type. I think it should be restricted to string | symbol. JavaScript coerces number values to strings when used as object properties. It does the same for anything other than a string or symbol:

1         => '1'
undefined => 'undefined'
{}        => '[object Object]'
() => {}  => '() => {}'
[]        => ''
/regex/   => '/regex/'

Referencing object keys this way is generally a bad idea and bug-prone, so I'd rather explicitly reject anything other than a string or symbol.

[kenkunz]

@ivanhofer I have a couple other areas for feedback / clarification, but I'll pause for now and add some additional comments tomorrow. Let me know if you have any questions or feedback on the items above. Thanks!

[ivanhofer]

@kenkunz thanks for your detailed responses.

typescript files

@ivanhofer replying to your earlier question:

do you have the intention to convert all files to TypeScript?

Not at this time. I don't have much experience with TypeScript – I've dabbled with it just enough to get a feel for it, and find that I prefer vanilla JavaScript. (My favorite language is Ruby – a very dynamic language that intentionally separates the notion of class from type ("duck typing" – if it quacks like a duck, it is one).

I personally find it easier to write the type definitions directly in the source code. It offers a instant feedback loop, because you see when an implemetation collides with the types.
Right now you could write any type definition inside the index.d.ts. file and you wouldn't really know if they are correct unless you test it with the same examples you have in the test.js file. Also for future changes, you always have to double-check if you also need to change the type-definitions. In a .ts file you would see the error directly when you change the implementation.
Maybe I miss something but I don't know a way how to make sure the d.ts files are up-to-date.
Setting up the whole project with TypeScript buildstep would would not be that much of an effort. I have some experience with a more complicated setup with my i18n library typesafe-i18n. I could assist you with that.
But I also can understand if want you keep it JS-only.

strict mode

I haven't thought about the usecase where someone could leave out the "final" state.
A "normal" and a "strict" option would make sense, but would probably be additional maintainance effort.

fallback state

Is the "*" new? I haven't seen it before.
I can see why it is useful to have a fallback state, but does it make sense to initialize the StateMachine with a not well defined state?
We could probably rewrite the initial state type to allow any value only if there is a "*" inside the state definition. What do you think?

number type

Yeah, I wasn't really sure about the number type. I added it because a key of a Record can be either string, Symbol or number. I have removed it!

tests

Regarding testing that the type definitions are correct, we could create an own test.ts file that only checks if TypeScript would allow it or if it fails:

const simpleSwitch =
	fsm('off', {
		off: { toggle: 'on' },
		on: { toggle: 'off' }
	})

simpleSwitch.toggle() // should work
// @ts-expect-error
simpleSwitch.turnOff() // not-allowed

For more advanced tests, something like ts-snippet could be used. I haven't used it yet. But mybe it is something that would fit here.

[ivanhofer]

There still is a TODO from my side:
https://github.com/kenkunz/svelte-fsm/pull/1/files#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8R39
What happens if you are in a state and call an action from another state?
I assume the state machine will just do nothing (the action does't throw an error). Right?

[kenkunz]

typescript files

You make some very good points. Will consider making this change when implementing a new feature or change to the API.

[kenkunz]

strict mode

I agree this would probably increase maintenance. Hopefully it could be modularized to minimize overhead.

[kenkunz]

fallback state

I added this in 0.7.0 (published 11/6). It's not currently highlighted in any of the examples.

We could probably rewrite the initial state type to allow any value only if there is a "*" inside the state definition. What do you think?

That's a great suggestion if it's not overly complicated. I can think of no reason you'd want to initialize an FSM in a state that has no actions.

[kenkunz]

tests

I didn't know about the @ts-expect-error directive – that's cool!

[kenkunz]

optional actions

What happens if you are in a state and call an action from another state?
I assume the state machine will just do nothing (the action does't throw an error). Right?

That's correct. I rely on this pattern extensively in UI code – attach an FSM-based hander machine.foo to an HTML element, only do something if the machine is in a state that implements the foo action (or there's a fallback action).

Essentially, this means the default behavior for any action is a no-op. We could choose to make this explicit – you'd need to add a no-op to the fallback state, or maybe support null to imply no-op. But that would add a lot more boilerplate – I tend to prefer the implicit no-op.

This feature – combined with the fact that the resulting state is returned from event invocations – yields an interesting side-effect. Assuming get is not defined on your FSM, machine.get() returns the current state (or, e.g., machine.currentState()).

This is more of an accident than an intended feature (I don't think I'm relying on it anywhere). I don't think the type-checking needs to allow invocations for actions that are not defined on any state (or fallback). If someone really wants to leverage this and not get a warning or type error, they can add a get no-op to the fallback state.

[kenkunz]

LifecycleAction type

I had a question about the LifecycleAction type declaration

From the docs:

A somewhat special case is when a new state machine object is initialized. The _enter action is called on the initial state with a value of null for both the from and event properties, and an empty args array. This can be useful in case you want different entry behavior on initialization vs. when the state is re-entered.

So the following

const machine = fsm('foo', {
  foo: {
    _enter: console.log
  }
});

…immediately produces { from: null, to: 'foo', event: null, args: [] }.

Is anything special needed in the type declaration to allow null for from and event? I'm not seeing any IDE warnings, but just wanted to confirm.

[ivanhofer]

LifecycleAction type

I now have added the "*" fallback and refactored all types a bit.

LifecycleAction type

The console.log example above works because it is typed as (...data: any[]) => void. Should I also add the behaviour of the "initial" call to the type definitions?

tests

How should we proceed? Do you want me to setup some basic .ts tests and then you can add some more? (I never have used StateMachines before so I don't really know good examples to test with)

[kenkunz]

fallback state

I now have added the "*" fallback and refactored all types a bit.

👍

LifecycleAction type

The console.log example above works because it is typed as (...data: any[]) => void. Should I also add the behaviour of the "initial" call to the type definitions?

I think it's a good idea – I'd like to avoid errors/warnings for intentionally supported use cases.

tests

How should we proceed? Do you want me to setup some basic .ts tests and then you can add some more? (I never have used StateMachines before so I don't really know good examples to test with)

You've done a ton of work already – why don't I take the first stab at this. I'll let you know if I have any questions.

Thanks!

[kenkunz]

I pushed my first pass at type declaration tests – see 076b228

• moved unit tests to test/units.js
• added test/types.js (don't want the compiled .ts to overwrite existing unit test file)
• added typescript to devDependencies
• updated scripts in package.json – targets include test:units, test:types and test (runs both sequentially)

Let me know if you have any feedback on this overall structure, as well as feedback on the type tests themselves.

Thanks!

[ivanhofer]

will take a look at your changes by the end of this week 👍

[ivanhofer]

Hi @kenkunz,
I couldn't get the return type of an action to only include NoOp when the action is not present in all states, so I added it everywhere.

Your tests look good. I have added a few tests that detect if a wrong type of argument was passed to an action.
From my side, I think this PR is completed now :)
Let me know what you think

[ivanhofer]

One minor suggestion: we could add a GH Action to run tests on every push to master. So you would get notified if you forgot tu run the tests before pushing.
It's not that hard to configure. You can tae a look how I do it in one of my projects:

• push to master: https://github.com/ivanhofer/typesafe-i18n/blob/main/.github/workflows/release.yml
• PRs: https://github.com/ivanhofer/typesafe-i18n/blob/main/.github/workflows/pull_request.yml

[kenkunz]

Hi @ivanhofer,

Regarding optional actions / noOp – the way I described this may have been somewhat confusing. When you invoke an event on an fsm in a state that doesn't include a matching action, the fsm internally does nothing (noOp), but the event invocation still returns the current state (string or Symbol). Sorry for the confusion! I'll take a look at updating the type declaration to match this expectation.

Nice additions to the type tests 👍.

I agree – the PR is looking good! I'll address the one item mentioned above and make some small formatting changes (spaces v. tabs and semicolons). I'm not super opinionated about these things other than valuing consistency. I haven't added prettier or eslint to this project yet … maybe that would be a good idea now that others are contributing :)

I should be merging this within the next day or so. I have a few other minor changes to incorporate alongside this (minor bug fix and README updates), after which I will publish v1.1.0 (should be within the next few days). Per your suggestion above, I'll also take a look at adding a GH action.

Thank you again for all your work on this! svelte-ism seems to be getting some adoption … this will greatly improve the developer experience. It's been great collaborating with you.

-Ken

[kenkunz]

@ivanhofer – regarding:

When you invoke an event on an fsm in a state that doesn't include a matching action, the fsm internally does nothing (noOp), but the event invocation still returns the current state (string or Symbol). Sorry for the confusion! I'll take a look at updating the type declaration to match this expectation.

… if you have a moment, take a quick look at 6c3a801 to see if this looks correct (sorry… TypeScript n00b!). Type tests are passing… so I'm cautiously optimistic I didn't screw this up 🤞.

[ivanhofer]

Ah sorry, I was confused by the NoOp. I haven't tried running an example or else I would have seen that the actual state get's returned :)

@kenkunz your last commit looks good.

Happy to help! Just let me know if you need help in the future ;)
In the past I always resisted using state machines in my code because existing libraryies were to big. (I don't want to include additional 20kb to a 30kb project, just for using a state machine).
Whenever I see a use-case for state machines I'll reach to svelte-fsm in the future!