xstate icon indicating copy to clipboard operation
xstate copied to clipboard

[docs] Verify TypeScript examples in docs

Open VanTanev opened this issue 3 years ago • 14 comments

This is an addendum to #2234. Sorry that I have the original commits in this PR, couldn't figure out a better way to do it.

Verify and fix all TypeScript examples in docs

This PR also introduces yarn verifyTypescriptExamples command that checks every piece of TypeScript code in the docs and runs it against the TS compiler.

VanTanev avatar May 23 '21 20:05 VanTanev

⚠️ No Changeset found

Latest commit: e0e0c50bead9ff47084a1d63b1992b919d8fa15d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar May 23 '21 20:05 changeset-bot[bot]

https://github.com/davidkpiano/xstate/pull/2235/commits/3d86fed43b46a2a7caf5e2011e3198736af6bbd1 and https://github.com/davidkpiano/xstate/pull/2235/commits/433887fd24ad9219c8da528fbaf685e35aa9a241 fix the errors the script found.

Now the output is:

➜  xstate git:(doc/fix-typescript-guide) yarn verifyTypescriptExamples                                            
yarn run v1.22.10
$ find packages docs -iname '*.md' -not -path '*/node_modules/*' -not -path '*/.vuepress/*' -not -iname 'CHANGELOG.md' -exec scripts/verifyTypescript {} \;
docs/guides/context.md:389.ts (396,21): Cannot find name 'CounterContext'.
docs/guides/context.md:389.ts (396,37): Cannot find name 'CounterEvent'.
Done in 17.15s.

VanTanev avatar May 23 '21 22:05 VanTanev

This is ready for review.

VanTanev avatar May 24 '21 11:05 VanTanev

Should most (all?) example code be rewritten in TS? I did that for context.md (not published) but then stopped because I'm not sure if this is desirable - I mean, it is for me, but not sure in general?

I'll publish the context.md update if everyone agrees we should move towards using TS for all example code.

VanTanev avatar May 24 '21 15:05 VanTanev

Should most (all?) example code be rewritten in TS?

@VanTanev I think so because:

  • types provide context for the examples even if you don't end up using them yourself
  • it's significantly easier to read ts and understand which parts to remove if using js than it is to read js and mentally keep track of what all of the things are supposed to be

It'd also be fairly easy to hook up a remark plugin like remark-typescript to add the ability to switch between them if we wanted or something.

with-heart avatar May 24 '21 15:05 with-heart

OK, added the condext.md TS rewrite!

VanTanev avatar May 24 '21 16:05 VanTanev

I also added the ability to verify JS, but have not fixed the errors that show up:

$ VERIFY_JS=1 yarn verifyCodeExamples                                                                                                                                                                            
yarn run v1.22.10
$ find packages docs -iname '*.md' -not -path '*/node_modules/*' -not -path '*/.vuepress/*' -not -iname 'CHANGELOG.md' -exec 
...
Found 686 issues
error Command failed with exit code 1

VanTanev avatar May 24 '21 16:05 VanTanev

q: would https://github.com/shikijs/twoslash provide similar functionality (and more)? I don't intend to block this PR - it is useful on its own and we don't want to pursue integrating twoslash right away. Just thought that maybe you have already looked into it - because I didn't have much time to do that myself - and that maybe you would already have some opinions on it that could be shared.

Andarist avatar Jun 14 '21 20:06 Andarist

q: would https://github.com/shikijs/twoslash provide similar functionality (and more)?

It does!

However, it wasn't released at the time I wrote this PR, and integration seems non-trivial. Do you think the PR is worth merging as is, or should I try to instead integrate twoslash?

VanTanev avatar Jun 15 '21 07:06 VanTanev

This is ready for re-review. I think it would be useful to merge as is and consider moving to twoslash in the future when someone has the time to do that.

VanTanev avatar Jun 15 '21 10:06 VanTanev

My only question now is how approachable these examples are going to be to those using JavaScript, and not TypeScript? Should we find a way to allow developers to toggle between the two?

Let's leave that for a separate PR. There are fixes in here that will be useful to xstate users today. We can strive to make it even better tomorrow.

VanTanev avatar Jun 15 '21 12:06 VanTanev

My only question now is how approachable these examples are going to be to those using JavaScript, and not TypeScript? Should we find a way to allow developers to toggle between the two?

Let's leave that for a separate PR. There are fixes in here that will be useful to xstate users today. We can strive to make it even better tomorrow.

It will help some users but will confuse others, see here: https://twitter.com/DavidKPiano/status/1404806818832977927

I want to be cautious about merging this unless we can satisfy both JS and TS users, and I think the best way to do it would be to show JS and TS together, via something like this: https://github.com/phryneas/remark-typescript-tools

We cannot merge this to main as-is unless we have both languages.

davidkpiano avatar Jun 15 '21 14:06 davidkpiano

OK. I don't have the bandwidth to do both right now, not sure when I will.

VanTanev avatar Jun 15 '21 15:06 VanTanev

OK. I don't have the bandwidth to do both right now, not sure when I will.

We can take this as a starting point and work with it; thanks so much for the work on this so far!

davidkpiano avatar Jun 15 '21 15:06 davidkpiano