Skip to content

Porting txstate #36

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

Closed
wants to merge 13 commits into from
Closed

Porting txstate #36

wants to merge 13 commits into from

Conversation

devanshj
Copy link
Contributor

@devanshj devanshj commented May 31, 2021

Superseded by #56

Todos

  • Add tests for top level on
  • Fix the _ bug - right now we need to add _: null is a state node only has a effect, I think the problem is at TS's end though will have to find a workaround. - update: made it on: null still haven't investigated yet to see if we can fix it completely we can't fix it, so pending is to add a custom error to tell user to add on: null
  • Integrate with twoslash for expectCompletionsToBe, expectQueryTextToInclude ie automated testing with language server
  • (note to self) type _Effect<D, P> = Effect<X, Y, X>, etc nope, doesn't make intellisense queries more readable
  • add jsdoc apparently there is no effect in adding jsdocs to the types, they get lost and the user can't see them
  • Typestated state
  • schema and new schema closes Suggestion: Less awkward way to pass context and event types #53

Original Description

Hi Cassio! Here's a little rough PR for porting txstate for better types.

Improvements

Way better error messages

Scenario 1


Before -
image
image

After -
image
image

Scenario 2


Before -
image
image

After -
image
image

And many more scenarios that I don't have handy...

Typed guards hence inferring context typestates

Example
// example picked from xstate docs
let [flightMachine] = useStateMachine<FlightMachineContext>({ trip: 'oneWay' })({
  initial: 'editing',
  states: {
    editing: {
      on: {
        SUBMIT: {
          target: 'submitted',
          guard: (context): context is FlightMachineSubmittedContext => {
            if (context.trip === 'oneWay') {
              return !!context.startDate;
            } else {
              return (
                !!context.startDate &&
                !!context.returnDate &&
                context.returnDate > context.startDate
              );
            }
          }
        }
      }
    },
    submitted: {}
  }
})

type FlightMachineEditingContext =
  { trip: 'oneWay' | 'roundTrip'
  , startDate?: Date
  , returnDate?: Date
  }
type FlightMachineSubmittedContext =
  | { trip: 'oneWay'
    , startDate: Date
    }
  | { trip: 'roundTrip'
    , startDate: Date
    , returnDate: Date
    }

type FlightMachineContext =
  | FlightMachineEditingContext
  | FlightMachineSubmittedContext

expectType<FlightMachineContext>(flightMachine.context);
if (flightMachine.value === 'submitted') {
  expectType<FlightMachineSubmittedContext>(flightMachine.context);
  
  if (flightMachine.context.trip === 'oneWay') {
    expectError(flightMachine.context.returnDate)
  }
}

Other notes

  • You can refer to this document to understand how the code works, I hope it helps xD.
  • Maybe more tests?
  • The PR is purposely opinionated at styles, naming, etc. Everything is negotiable.
  • No pressure to get this in, just wanted to know if you're interested.

@cassiozen
Copy link
Owner

Hi @devanshj - I'm still trying to dive into this, I have a couple of questions if you don't mind:

1 - You left pretty much all the types intact in index.tsx - but I'm assuming these aren't needed anymore?
2 - You have some helper types (great stuff, BTW) under the namespaces O, L, F and A. Why are they under so many namespaces?
3 - I'll be honest with you - I really want to use this, but I don't feel like I can contribute right now to your project. So, my question is - can you keep helping with changes until we push version 1.0? Here are the expected changes:

  • New event system with payload (Already merged)
  • Top-level events/transitions (Top level events #41)
  • New effects parameter signature with access to context (Access context inside effect #33)
  • Nested/hierarchical states (Nested/hierarchical States #21)
    If you can and are interested we can coordinate changes on how to change the implementation and types for these changes, and by then I think I'll be more prepared to make changes into txstate directly.

@devanshj
Copy link
Contributor Author

@cassiozen Before I reply to you I think there's a big misunderstanding xD You realise this PR has nothing to do with txstate per se, I'm calling it "Porting txstate" because I'm using the techniques used in txstate. This PR and txstate are TOTALLY independent. I linked that document because it'll help you understand this PR somewhat.

So I'm not asking you to contribute to txstate, this PR is to be treated independent as if txstate does not exist.

Did you already know all this or there was some misunderstanding here? xD

@devanshj
Copy link
Contributor Author

To answer your questions -

You left pretty much all the types intact in index.tsx - but I'm assuming these aren't needed anymore?

The thing is I've kept the user facing types and types for implementation different. The reason is you can't use the one's I've added (I'm calling it user facing) to type implementation details (Let me know if you want me to elaborate on this). So to answer your question we'll be needing both of them.

You have some helper types (great stuff, BTW) under the namespaces O, L, F and A. Why are they under so many namespaces?

It's inspired from ts-toolbelt. The idea is all types under O namespace serve as operations on object, L on arrays/tuples, F in functions and A on all types. It's just a matter of convention and preference.

can you keep helping with changes until we push version 1.0?

More or less yes, I can't give a word because I haven't checked the issues yet but it's mostly a yes from me. The worst case scenario is we'd have to compromise on types in scenarios where it's not possible to type them. I'll give you a concrete answer once I check issues you've linked.

@cassiozen
Copy link
Owner

Yes, I understand that, my writing was confusing. Whenever I say "contributing to txstate" I mean "contributing to 'types.ts' inside this project".

@devanshj
Copy link
Contributor Author

Yes, I understand that, my writing was confusing. Whenever I say "contributing to txstate" I mean "contributing to 'types.ts' inside this project".

Ah gotcha, I too thought maybe you meant this but wasn't sure xD

@devanshj
Copy link
Contributor Author

devanshj commented Jun 16, 2021

I'll give you a concrete answer once I check issues you've linked.

I went through all four issues seems doable to me. So it's a yes I'll keep updating this PR (or opening new one's if we're merging feature by feature) till 1.0 (or even forever if time permits :P)

Just there's one issue that the types assume that users are writing the machine definition (config) in place as literal so that the type inferred of the definition is of unit type (the type which can be assigned only one value), as in anything like the following might create an issue...

let m = { ... } // the definition here
useStateMachine()(m)
// won't work because the types of `m` are already inferred
// so we can't have contextual inference which might cause problems
// because things in `m` would be inferred as `string` (which makes the type non-unit)
// instead of literals like "red"` or whatever

useStateMachine()({
  initial: Math.random() > 0.5 ? "a" : "b", 
  states: { a: {}, b: {} }
})
// might be a problem because the type of machine definition inferred
// is not unit it's a union of two types one with "a" and other with "b".

I haven't investigated how to go about cases like this but this could be a potential blocker for this PR, imo the pros are way more than this one con, but you're the boss :P

@devanshj
Copy link
Contributor Author

@cassiozen Before I reply to you I think there's a big misunderstanding xD You realise this PR has nothing to do with txstate per se, I'm calling it "Porting txstate" because I'm using the techniques used in txstate. This PR and txstate are TOTALLY independent. I linked that document because it'll help you understand this PR somewhat.

So I'm not asking you to contribute to txstate, this PR is to be treated independent as if txstate does not exist.

Did you already know all this or there was some misunderstanding here? xD

Also @cassiozen I realized this sounded a little rude when I reread later, I wasn't in the best headspace when I replied, my sincere apologies 🙏😅

@cassiozen
Copy link
Owner

No, you didn't sound rude at all!

Ok, so we have everything merged on the main branch for the 1.0 release, whenever you have time to update the types, we can release it as is and leave hierarchical state machines for next on queue.

A few comments on your responses:

User facing types: Ok, perfect. It worries me a little bit that future developments (like the hierarchical state machines) will have to be done on both sides. Maybe there is a way for me to reuse some of the ported txstate types as user-facing?

Namespaces: Got it, makes sense. In the current types, I moved from single-letter to fully named generics - Do you have strong feelings about this? I could refactor the types.ts names, keeping the same structure.

@devanshj
Copy link
Contributor Author

devanshj commented Jun 18, 2021

Cool, going to work on types now hopefully I'll update the branch soon.

Maybe there is a way for me to reuse some of the ported txstate types as user-facing?

In theory, yes. Thought it'll be extremely verbose and perhaps would do more harm than good, like you'd need more assertions in your implementations.

By verbose I mean it'll look like...

const someImplementationDetail = <N, C>(node: MachineDefinition.StateNode<N, C, []>) => {}

Or in future versions like...

const someImplementationDetail = <N, C>(node: MachineDefinition.StateNode<N, C, MachineDefinition.Precomputed<N>, []>) => {}

Meaning there would be no handy generic-free type that you'd be able to assign to a variable and call it a day.

Instead what I would suggest is dumb down the implementation types. Make them generic-free and just "runtime equivalent" like so...

interface StateNode {
  initial: string,
  states: Record<string, StateNode>
}

Because there isn't a point of having sophisticated types for implementation, in my opinion. If you could manage to have implementation types as good as user facing, well and good, though I personally wouldn't care so much.

Do you have strong feelings about this?

I kinda have, though we're more or less on the same page, I too am for fully named generic but in few cases I prefer them to be single letter. For example right now more or less all types start with D, C, S or at least D, C. It wouldn't make sense to write Definition, Context, Self all the time. Another instance is for the types inside O, L, etc namespaces there too it's obvious what single generics types mean. Otherwise I too am for fully named generics like you'll see States, Gaurd, etc in the types.ts. Though in some places they are a little ambiguous like Ts, S, etc. and I'll refactor them myself.

@devanshj devanshj marked this pull request as draft June 21, 2021 18:18
@devanshj
Copy link
Contributor Author

devanshj commented Jun 21, 2021

Hi @cassiozen here's some wip, I'll keep updating the original description and ping you whenever I do it (not sure if github notifies for new commits in a PR), still 15% work more to go

Could you go through the test when you get time? I'm doing some fancy stuff (you'll know when you see the tests :P) so want to make sure that's in line with you.

Also I've bailed out of tsd because

  1. expectError doesn't give me live feedback whereas @ts-expect-error does, moreover it isn't reasonable to have expected red underlines floating around, it's like now you don't know if something is off even if there's red shit in your ide because it could include the expected ones.
  2. expectType too doesn't give live feedback (the way it's typed it only checks for subtyping afaik so you'll have to run tsd to actually see the results) whereas A.test(A.areEqual<A, B>()) does give me live feedback

So now just tsc tests the types

@cassiozen
Copy link
Owner

Yes, I'm super curious to check it out. I'm a little blocked because of worked but will check as soon as possible.

As for TSD, that's totally fine. I wanted a way to test the types and make sure we make no regressions during new development - but if you can test them using something else that's fine.

@devanshj
Copy link
Contributor Author

Yeah no worries take your time it's unpaid work after all ;)

@cassiozen
Copy link
Owner

@devanshj - I just went through the tests and through the initial work on types.ts. So far everything looks good. I love how you use string literals instead of never to represent errors in the type level.

I plan to start tackling the code to use primitive types in the end of next week.

@devanshj
Copy link
Contributor Author

devanshj commented Jun 24, 2021

Nice, I too will complete the rest todos soonish!

I love how you use string literals instead of never to represent errors in the type level.

Haha ikr thanks!

Also if I can't find a workaround for the "_" thing would it be a blocker? Or it's fine to expect users to write _: null? Or would you want to compromise on some features to have users not write _: null?

@cassiozen
Copy link
Owner

Wait, I don't understand. Is that for states with no transitions?

@devanshj
Copy link
Contributor Author

devanshj commented Jun 28, 2021

Yep, if effect is the only property (ie no on) you'd have to add a phantom _: null to make it work 😬 (related ts issue)

@cassiozen
Copy link
Owner

I kind of understand the issue, but it's definitely beyond my TS knowledge. Question, instead of the phantom _, does it work if the user is required to provide a null on key, or a final state like type: 'final'?

@devanshj
Copy link
Contributor Author

devanshj commented Jun 30, 2021

Yep, both would work. Let me know if you want to go with any one of those.

@cassiozen
Copy link
Owner

"type: final" would have different implications, so I Think the least bad would be an "on: null".

@devanshj
Copy link
Contributor Author

devanshj commented Jul 3, 2021

Also in case you suggested on: null thinking it has to be null then no, on: {} works too. Should we keep that? Or both?

@cassiozen
Copy link
Owner

I think we could keep both.

@devanshj devanshj marked this pull request as ready for review July 12, 2021 00:20
@devanshj
Copy link
Contributor Author

devanshj commented Jul 12, 2021

I think this is ready for final review and merging!

Also I kept it as on: {} and not on: null, because allowing the latter seems to be that we're changing runtime semantics unnecessarily for type-land issues. And I also added a custom error that instructs the user to add on: {} when required you can see the tests for more.

Don't announce v1.0 yet, I think I'm gonna make a nice document to show off all these features :P which can go with the announcement. I hope I get time and I'll let you know in either case.

Let me know if you'd first like to make the runtime implementation changes in a new branch and get it merged with main and then have me rebase it to that, I think that'd be more clean and then the CI would also pass on this PR.

@cassiozen
Copy link
Owner

That is awesome, @devanshj - you're the best!

So my plan is to release this as a beta for at least a week, so you'll have time to write the nice document!

I also want to do a clean-up pass on index.tsx to remove what now are unnecessary type definitions and either use primitives or try to use some TXState types passing some anys to fill in the generics.

@devanshj
Copy link
Contributor Author

That is awesome, @devanshj - you're the best!

Haha thanks a lot, my pleasure! And thanks to you too for being so welcoming!

So my plan is to release this as a beta for at least a week, so you'll have time to write the nice document!

Great, I think a week would be sufficient time.

I also want to do a clean-up pass on index.tsx to remove what now are unnecessary type definitions and either use primitives or try to use some TXState types passing some anys to fill in the generics.

Yep, feel free to ask questions if you're trying to reuse the types.

@cassiozen
Copy link
Owner

Wait, it seems to be something wrong: I can't find the createSchema function...

@devanshj
Copy link
Contributor Author

devanshj commented Jul 12, 2021

That's right I replaced it with a more minimal t function because the user is going to write them many times so it doesn't make sense to have such long name imo. Oh oops I realised I forgot to update the runtime tests haha.

Also there's some error in exit events, I'll fix it tomo.

@devanshj
Copy link
Contributor Author

devanshj commented Jul 26, 2021

Also you haven't addressed this. And I have added some tasks here too. (I'll probably make type changes in this branch)

@cassiozen
Copy link
Owner

Should we close this PR in favor of #56 ?

@devanshj
Copy link
Contributor Author

Yep we should, closing.

@devanshj devanshj closed this Jul 26, 2021
@devanshj
Copy link
Contributor Author

Tho this is left here

@devanshj devanshj mentioned this pull request Jul 26, 2021
13 tasks
@cassiozen
Copy link
Owner

Let me address those separately:

useStateMachine()({
  initial: Math.random() > 0.5 ? "a" : "b", 
  states: { a: {}, b: {} }
})
// might be a problem because the type of machine definition inferred
// is not unit it's a union of two types one with "a" and other with "b".

Absolutely OK.

let m = { ... } // the definition here
useStateMachine()(m)
// won't work because the types of `m` are already inferred
// so we can't have contextual inference which might cause problems
// because things in `m` would be inferred as `string` (which makes the type non-unit)
// instead of literals like "red"` or whatever

I'm ok with this one as well, though we might want to explicitly document this somewhere.
Question: Would it work if the user defined the configuration externally but used as const everywhere?

@devanshj
Copy link
Contributor Author

devanshj commented Jul 26, 2021

though we might want to explicitly document this somewhere.

We'd document both, I'll add it to the "downsides" section of my release article.

Would it work if the user defined the configuration externally but used as const everywhere?

Yep that'd work, though consequently the errors will be much harder to read.

What we can do is we'd also provide an identity function called createDefinition then users can use it like this -

let d = createDefintion({ ... }) // more readable errors and no need for "as const"
useStateMachine(d)

Wdyt?

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.

Suggestion: Less awkward way to pass context and event types
3 participants