Skip to content
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

Porting txstate #56

Merged
merged 46 commits into from
Jul 27, 2021
Merged

Porting txstate #56

merged 46 commits into from
Jul 27, 2021

Conversation

devanshj
Copy link
Contributor

@devanshj devanshj commented Jul 12, 2021

Supercedes #36

I was trying to write some types for implementation for you so I even rewrote the whole thing because I felt like and I was bored ahaha. Take no pressure in merging this but if you like it do it and you can also use the types from here.

There's some really good stuff in here that is branded types and assertNever for exhaustive check and strongly typed record which I don't have time to explain because I'm going to sleep :P but the gist is as all primitive are branded you can't make mistakes like these...

definition.states[event.type] // nope
definition.states["foo"] // nope
R.get(definition.states, machineState.value) // the only way to access the node
R.get(definition.states, transition.target) // also works till the time the type is the same

let x = machineState.value;
x = event.type // nope both are string in runtime but aren't of the same type so can't be assigned.

Haven't checked if the tests are passing or not btw didn't have time was in a rush. Coz really I just rewrote this for fun don't mind me xD (this branch is based on txstate-port btw, if you want to quickly check the diff you can do it here). And if it interests you feel free to ask questions.

And also if you like those Impl types let me know if you want me to add them to txstate-port branch itself.

To-do:

  • Rebase changes from "Porting txstate" branch
  • Explicit initial event (Suggestion: Have an explicit initial event? #60)
  • Fix tests
  • Fix issue with guards
  • See if we can have types ContextImpl instead of Context.Impl
  • Update Docs
  • Update Examples
  • Devansh's release article
  • Cassiozen's release video
  • Enforce usage of t
  • Check what happens if the definition is not literal.
  • lite-typed version
  • nextEvents.includes inferred as never (nextEvents.includes inferred to never #62)

@cassiozen
Copy link
Owner

Hey @devanshj - I just wanted to say that this is awesome! I'm on crunch time at work so I didn't have time to review It yet, but I'm super excited to merge!

@devanshj
Copy link
Contributor Author

Thanks I'm glad you liked it! I didn't even expect you to want to merge it haha. And no worries at all take your time, I understand.

@cassiozen
Copy link
Owner

cassiozen commented Jul 23, 2021

Ok, I started reading the code. I will probably more questions as I go ;)

  1. Is you extras/R kind of a tiny TS Rambda of sorts?
  2. This version doesn't yet support nested states, correct?

Some other observations & things to do:

  1. TSDX (the setup tool used in use-statemachine) doesn't support TypeScript 4 yet, but there's a workaround using yarn resolutions - I updated the project to use it, so make sure to use yarn from now on.
  2. This version doesn't have a specific log for when a transition is blocked by a guard and doesn't log successful transitions. I can work on that.
  3. The minimized bundle is now bigger than half a kb (even using brotli instead of gzip), so maybe we should rename the catch-phrase from "The ½ kb state machine hook for React" to "The <1 kb state machine hook for React" 😂
  4. Loved assertNever.
  5. Need to update examples and failing tests. I can also work on those
  6. Need to update docs and work on a release article. Can we split the work on this? Do you have the availability?

Overall I love how this is going. You put a lot of effort into this, so I'd like to invite you to be a maintainer of this project with me. Is this something that interests you?

@devanshj
Copy link
Contributor Author

Is you extras/R kind of a tiny TS Rambda of sorts?

Not really, R stands Record. It only has functions you'd use on an object used as a map. So instead of a[k] you'd write R.get(a, k) and instead of Object.keys(a) you'd write R.keys(a). The reason is that typescript doesn't allow indexing via branded string, so now { [_ in string & { __someTag: true }]: "foo" } is actually { __K: string & { __someTag: true }, __V: "foo" } instead. Let me know if you want me to elaborate

This version doesn't yet support nested states, correct?

That's right

TSDX (the setup tool used in use-statemachine) doesn't support TypeScript 4 yet, but there's a workaround using yarn resolutions - I updated the project to use it, so make sure to use yarn from now on.

Sure

This version doesn't have a specific log for when a transition is blocked by a guard and doesn't log successful transitions. I can work on that.

Ah must have missed, thanks.

The minimized bundle is now bigger than half a kb (even using brotli instead of gzip), so maybe we should rename the catch-phrase from "The ½ kb state machine hook for React" to "The <1 kb state machine hook for React" 😂

Haha still has a lot of useless stuff like assertNever can be an empty function we need not throw the error it's only because we get compile-time exhaustive checks.

Loved assertNever.

Haha thanks, idr if it was original or inspired xD

Need to update examples and failing tests. I can also work on those

Yep that'd be good.

Need to update docs and work on a release article. Can we split the work on this? Do you have the availability?

Sure thing, I'd prefer to work on a release article. I was thinking of hosting it somewhere (as opposed to md) so we can have twoslash intergration (as opposed to screenshots) but then I think that'd be too much of work :P wdyt?

You put a lot of effort into this, so I'd like to invite you to be a maintainer of this project with me. Is this something that interests you?

I understand where you're coming from but it's not something that I'd opt into majorly because this is something I do for fun quite literally and maintaining a library is no fun xD Look I understand that contribution comes with an implicit responsibility which I'll gladly fulfil (eg helping you out if users report some ts issues etc) but "maintainer" is a overhead responsibility for me especially given the area this library focuses is not really of my interest, I've never written a state machine nor have I even used xstate, it just happened to be a typing challenge that I took. So thanks for the offer but I don't think I'll be able to take it sorry!

src/extras.ts Outdated Show resolved Hide resolved
@devanshj
Copy link
Contributor Author

Other things pending here -

  1. rebase to txstate-port (there are some changes I did)
  2. See if we can have types ContextImpl instead of Context.Impl because the intellisense quickinfo tooltip shows the type of everything as Impl which is hella annoying and useless

src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated Show resolved Hide resolved
src/extras.ts Outdated Show resolved Hide resolved
@cassiozen
Copy link
Owner

So thanks for the offer but I don't think I'll be able to take it sorry!

Don't worry, I understand.

Sure thing, I'd prefer to work on a release article. I was thinking of hosting it somewhere (as opposed to md) so we can have twoslash intergration (as opposed to screenshots) but then I think that'd be too much of work :P wdyt?

I think you should absolutely do it. Is there any public platform that supports twoslash (like dev.to)?

@devanshj
Copy link
Contributor Author

Don't worry, I understand.

Thanks for understanding!

Is there any public platform that supports twoslash (like dev.to)?

I doubt. We can use gh-pages I guess. I could use remark-shiki-twoslash or something like that to output html.

src/extras.ts Outdated Show resolved Hide resolved
@cassiozen
Copy link
Owner

Question: Prettier is screaming at me when I open the files. I see that you like to be precise about the formatting - can you live with some configuration of Prettier? We can disable it too, but I would prefer to keep it.

@devanshj
Copy link
Contributor Author

devanshj commented Jul 23, 2021

I see that you like to be precise about the formatting

Good observation :P I was about to mention about prettier, I tell you what -- I absolutely hate it. I could write an essay on how it's just wrong. But hey it's your project if you prefer to keep it do that. Though I doubt it'd matter as it's not like a big org where people would have diff opinion about formatting and would bike-shed all the time (that's the only place I can give prettier a benefit of doubt). Though again I'd have you take the call.

Oh wait yeah I would still want prettier to ignore types.ts at least or it'd make things absolutely unworkable.

@devanshj
Copy link
Contributor Author

Did you rebase to the latest txstate-port?

@cassiozen
Copy link
Owner

I tell you what -- I absolutely hate it. I could write an essay on how it's just wrong

🤣😆

I'd have you take the call. (...) I would still want prettier to ignore types.ts

Deal - I'll keep ignoring types.ts and I left the prettier requirements less strict, but I'll keep on index and extra.

@cassiozen
Copy link
Owner

Hey @devanshj - Do you think this is in a state we could release another beta before the official 1.0 release? It could be nice to have some additional user testing.

@cassiozen
Copy link
Owner

Found a bug while converting the timer example. Filled separately for better organization: #62

@devanshj
Copy link
Contributor Author

Do you think this is in a state we could release another beta before the official 1.0 release? It could be nice to have some additional user testing.

Sure thing but not yet, I'll mark this a draft for clarity and will open once it's ready or at least ready enough for a beta. Right now enforcing t and disallowing $$initial as an event type are remaining then we'd be ready for another beta.

@devanshj devanshj marked this pull request as draft July 26, 2021 17:11
@devanshj devanshj changed the title Unsolicited rewrite :P No pressure to merge Porting txstate and Unsolicited rewrite :P No pressure to merge Jul 26, 2021
// match output dir to input dir. e.g. dist/index instead of dist/src/index
"rootDir": "./src",
// stricter type-checking for stronger correctness. Recommended by TS
// "rootDir": "./src",
Copy link
Contributor Author

@devanshj devanshj Jul 26, 2021

Choose a reason for hiding this comment

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

(from txstate-port) I think removing "rootDir": "./src" will affect the build (?), but I want to type-check "tests" folder too. How to go about this?

@cassiozen cassiozen changed the title Porting txstate and Unsolicited rewrite :P No pressure to merge Porting txstate Jul 26, 2021
@devanshj
Copy link
Contributor Author

@cassiozen I think this is good for another beta.

@cassiozen cassiozen changed the base branch from main to 1.0.0-beta July 27, 2021 05:01
@cassiozen cassiozen marked this pull request as ready for review July 27, 2021 05:01
@cassiozen
Copy link
Owner

I think this is good for another beta

Awesome. I created a new branch 1.0.0-beta and rebased this PR to merge and bake a new release from the original repo.

We can continue working on this same PR on your fork, or open a new PR, whatever you prefer.

@cassiozen cassiozen merged commit e0fb47e into cassiozen:1.0.0-beta Jul 27, 2021
cassiozen added a commit that referenced this pull request Jul 27, 2021
@cassiozen
Copy link
Owner

@all-contributors please add @devanshj for bug, code, ideas and test

@allcontributors
Copy link
Contributor

@cassiozen

I've put up a pull request to add @devanshj! 🎉

@devanshj
Copy link
Contributor Author

The only thing pending other than documentation is lite-types, I think I'll open a new PR for that and base it on the beta branch. You can add commits changing documentation on the beta branch directly if you want.

cassiozen added a commit that referenced this pull request Jul 28, 2021
* Porting txstate (#56)

feat: better types via txstate port

* docs: add devanshj as a contributor for bug, code, ideas, test (#63)

* docs: update README.md [skip ci]

* docs: update .all-contributorsrc [skip ci]

Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>

* Contributors

* fix(nextEvents): only normalise don't widen (#64)

* Update README.md

Co-authored-by: Devansh Jethmalani <[email protected]>
Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com>
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.

2 participants