Skip to content

TypeScript Declaration File #165

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

TypeScript Declaration File #165

wants to merge 1 commit into from

Conversation

todd
Copy link

@todd todd commented Jun 18, 2019

Re: #158

Almost a year after I submitted the initial issue and six months after I took my first stab at this, I've finally got something somewhat useable (big yikes, sorry for the delay). This isn't ready for a merge just yet, but I wanted to get this open to solicit feedback.

Pinging @ifandelse, @miedmondson, and @king612 for their thoughts since they were the participants in the original thread.

A couple of notes:

  • This only covers BehavioralFsm for the moment as that's what I use primarily and it's the more complicated of the two classes. Should we be happy with this implementation, it should be easy enough to abstract and apply to Fsm.
  • I fear I may have gone a bit overboard with the type safety here in order to make things as inspectable as possible. I'm particularly interested in feedback on how usable folks think this implementation is - you can see how to use this in types/BehavioralFsmTests.ts, which implements the traffic light example from the docs.
  • If you've ever used TypeScript with React, some of the example implementation may look familiar to you. My implementation was heavily inspired by those typings.
  • An aside: the extensible nature of Machina through an options constructor made writing these fairly complicated and a bit brittle. That's not a criticism of the underlying library, just a fact that I wrestled with when putting this together. An abstraction or refactor of these to utilize ES6 classes could aid in the maintainability of these types. Not recommending we go this route (certainly not immediately, anyway), just making an observation.

Should we like where this is headed, I can clean this up a bit more, add the types and tests for Fsm, and plug the transpiler into the build pipeline so we ensure clean builds as part of CI. Additionally, anyone who would like write access to my fork to help work on these should feel free to reach out.

Let me know what you think, team.

@king612
Copy link

king612 commented Jun 18, 2019 via email

'*'?: TransitionHandler<T, S>;
_onEnter?: TransitionHandler<T, S>;
_onExit?: TransitionHandler<T, S>;
timeout?: TransitionHandler<T, S>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While "timeout" could be an event that a state handles, it's not any sort of special handler like *, _onEnter, or _onExit, so it's covered by line 26:
[handler: string]: TransitionHandler<T, S> | undefined;

Ironically, I think this mistake is probably traceable back to me.

[namespace: string]: {
targetReplayState: S;
state: S;
priorState: S;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetReplayState, state, and priorState are strings. state, priorState and currentActionArgs are optional.

As below, I think these mistakes are also probably traceable back to me.

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.

3 participants