-
Notifications
You must be signed in to change notification settings - Fork 146
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
base: master
Are you sure you want to change the base?
Conversation
Thank you for your work and the opportunity to comment, however we had to
move on to something else for our workflow solution. In general, the
coding on this project is some of the most impressive and advanced coding
I've seen, especially for JS / TS. But this also makes it very challenging
to understand what the package does just by reviewing the code, and the
documentation is very sparse, so I just didn't have confidence we were
going to be able to make it work. Also, the dearth of activity,
contributors, and releases was concerning. IMHO, to be successful, the
documentation can't just be some examples. I never could figure out what
the core operations of the package were, how they interacted, all the built
in behavior, the primary algorithms (conceptually), etc.
Very impressive piece of work, but I think you all would get a lot more
traction with it if you could work up a good, comprehensive white paper on
how it works at a higher level, the key components, how they interact, how
to implement a workflow with it, etc. It was the most promising package I
found after lots of searching for a JS based workflow module that was free
open source and very extensible.
Thanks again for your work, and good luck with the package.
John K.
…On Tue, Jun 18, 2019 at 2:23 AM Todd Bealmear ***@***.***> wrote:
Re: #158 <#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 <https://github.com/ifandelse>, @miedmondson
<https://github.com/miedmondson>, and @king612
<https://github.com/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.
------------------------------
You can view, comment on, or merge this pull request online at:
#165
Commit Summary
- 🚧 First Pass
File Changes
- *M* .gitignore
<https://github.com/ifandelse/machina.js/pull/165/files#diff-0> (1)
- *M* package.json
<https://github.com/ifandelse/machina.js/pull/165/files#diff-1> (7)
- *A* tsconfig.json
<https://github.com/ifandelse/machina.js/pull/165/files#diff-2> (63)
- *A* types/BehaviorialFsmTests.ts
<https://github.com/ifandelse/machina.js/pull/165/files#diff-3> (118)
- *A* types/index.d.ts
<https://github.com/ifandelse/machina.js/pull/165/files#diff-4> (61)
Patch Links:
- https://github.com/ifandelse/machina.js/pull/165.patch
- https://github.com/ifandelse/machina.js/pull/165.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#165?email_source=notifications&email_token=ABLDDUD23NOAO6RBWGS4OJLP3B5PFA5CNFSM4HY4WDC2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4G2BS7UA>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLDDUCRB5CLMBN5GKMN7CLP3B5PFANCNFSM4HY4WDCQ>
.
|
'*'?: TransitionHandler<T, S>; | ||
_onEnter?: TransitionHandler<T, S>; | ||
_onExit?: TransitionHandler<T, S>; | ||
timeout?: TransitionHandler<T, S>; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
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:
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 toFsm
.types/BehavioralFsmTests.ts
, which implements the traffic light example from the docs.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.