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

Add twoslash to docs & migrate contents #481

Merged
merged 32 commits into from
Jun 30, 2021
Merged

Add twoslash to docs & migrate contents #481

merged 32 commits into from
Jun 30, 2021

Conversation

frencojobs
Copy link
Contributor

@frencojobs frencojobs commented Jun 26, 2021

This PR does

  • Add proper Twoslash integration
  • Make line highlighting work properly
  • Add support for top-level await in code blocks
  • Convert as much JSX API to TSX (done 🎉)
  • Modularize the examples with Twoslash include blocks
  • Handle imaginary imports with twoslash's multi-file system
  • Add copy button to code blocks
  • Add syntax highlighting to type annotations?? (currently blocked by Twoslash)

Closes #447

Help needed

  • /parametrized-rendering contains two code blocks that don't compile
  • /ssr contains a code blocks that don't import correct API
  • Player API's first code block didn't complete all props so can't pass the compiler

Remarks

  • A few code blocks from API sections that only show type definitions and don't have bodies are excluded since they can't pass compiler to migrate
  • /composition's lazy import can't be correctly typed

@JonnyBurger
Copy link
Member

Woah this is great! Thanks a lot for kicking this off! Don't worry if we cannot convert everything 100% to twoslash, if we can merge those examples who work great it's already a great improvement! We can strip the parts that are too hard for now.

Let me know if I should help! I will in any case in a few days try it out and see where we stand :)

@frencojobs
Copy link
Contributor Author

Hi @JonnyBurger All docs except for what is mentioned in my remark are migrated to twoslash code blocks. But two items mentioned in the help needed are migratable but I couldn't get them right. Can you check them, please?

The first one is from parametrized-rendering,

import {Sequence} from 'remotion';
import {MyComponent} from './MyComponent';

export const Root = () => {
  return (
    <>
      <Sequence
        id="my-video"
        width={1080}
        height={1080}
        fps={30}
        component={MyComponent}
        defaultProps={{
          propOne: 'Hi',
          propTwo: 10
        }}
      />
    </>
  )
}

TSC is complaining that Sequence doesn't have these id, width, height etc props. Is that correct? Did you mistake it with composition? (I'm not sure about remotion APIs)

The second one is from ssr and TSC says this import doesn't exist.

import {evaluateRootForCompositions} from 'remotion';

We can leave them not migrated if TSC is wrong but can you check and confirm, please. Thanks!

@Iamshankhadeep
Copy link
Contributor

Iamshankhadeep commented Jun 27, 2021

import {Sequence} from 'remotion';
import {MyComponent} from './MyComponent';

export const Root = () => {
  return (
    <>
      <Sequence
        id="my-video"
        width={1080}
        height={1080}
        fps={30}
        component={MyComponent}
        defaultProps={{
          propOne: 'Hi',
          propTwo: 10
        }}
      />
    </>
  )
}

This should be <Composition/> I suppose as <Sequence /> does not have this props. @JonnyBurger

import {evaluateRootForCompositions} from 'remotion';

we don't have evaluateRootForCompositions this in our codebase. I checked just now.

BTW thanks for this @frencojobs, this PR is awesome.

@JonnyBurger
Copy link
Member

I just fixed the remaining snippets!
And I got to play around with it for the first time, this is absolutely brilliant!

Not only is it helpful for the reader, but it also ensures that we don't have typoes or unrunnable code in our documentation! I never knew we could have a type checker for our docs - now I never wanna go back! :)

For me it's good enough to merge, we can have e.g. the copy button in another PR. But I'll wait for you to have the final go!

@frencojobs
Copy link
Contributor Author

frencojobs commented Jun 28, 2021

Great! I have added exact copies of the default copy-to-clipboard buttons. I'll open this up to ready for review.

Sorry for delay, timezones you know. By the way, I'm not sure about how these Issuehunt stuffs work.

@frencojobs frencojobs marked this pull request as ready for review June 28, 2021 07:08
@frencojobs frencojobs changed the title WIP: add twoslash to docs & migrate contents Add twoslash to docs & migrate contents Jun 28, 2021
@frencojobs
Copy link
Contributor Author

OOMs on docusaurus build. We might have to wait for facebook/docusaurus#4997. I'm looking for a workaround in the meantime.

@frencojobs
Copy link
Contributor Author

Seems like trying to increase the memory only results in "rewarding for bad behavior" for this RAM eater. Since this is only because of the way docusaurus handle things (can confirm this, transforming the same files in eleventy which uses the same remark plugin only takes a portion of the time it took in docusaurus), I'll try to switch to another approach by twoslashifying markdown files beforehand feeding to docusaurus.

@JonnyBurger
Copy link
Member

I saw it too, performance is very poor too. Let's consider opening a (proper) issue on shiki-twoslash-docusaurus, maybe we can get some help from there too!

@frencojobs
Copy link
Contributor Author

There is this shikijs/twoslash#35 which was closed recently because, in fact, it is not twoslash's issue. The docusaurus plugin merely exports the same remark plugin again. The remark plugin is used in all of the plugins including gatsby, eleventy etc, and yet they still yield good if not great performances.

So I'm thinking in the meantime the PR in docusaurus upstream is in WIP (I'm not sure if it will help too), to write a script to convert all these .md files into .mdx files and stuff the converted twoslash code blocks as HTML blocks instead of letting docusaurus deal with it.

So we'll turn, this example.md into

# Markdown

\```ts twoslash
twoslash
\```

example.mdx with a script,

# Markdown

<pre> ... twoslash </pre>

Then we'll use the output directory as docs instead of default docs in production.

It should be fairly quick just to turn these blocks and docusaurus will still have normal performance.

@JonnyBurger
Copy link
Member

That's pretty clever! Thanks for pushing it further. Let's see if it works 🤔

If the CI fails with cb() not called, often it can be fixed by just retrying. Unsure why npm is shaky.

@frencojobs
Copy link
Contributor Author

@JonnyBurger Haha, sorry didn't use that technique aforementioned. I tried it for a day and gave up since mdx parser can't seem to be parsing stuffs correctly. But today I found a leak in twoslash and fixed it. It's soo much faster now than before. It is merely about the same performance as the normal docusaurus (at least on my end). Still waiting for the checks but I'm sure they'll pass. So it is mergeable now.

Also, because I wanted to make sure first, I added a script called format in the docs' script folder. I noticed that prettier can't format tsx/ts code blocks in markdown, and there are some inconsistencies in the code blocks such as tabs, semicolons etc, the script will help format the code.

It would be nice if you could review it too. If you think it's good, you can format the files yourself, it should work fine but will probably require some manual checks as to update line highlights etc.

Thanks.

Copy link
Member

@JonnyBurger JonnyBurger left a comment

Choose a reason for hiding this comment

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

This is so great! Absolutely flashed by the PR, there are so many nice details, bugs found, done with so much care! I'm in love with this PR... 🥰

Thanks a lot for making this, and going all the way, even finding and solving a bug in shikijs - incredible work!

@JonnyBurger JonnyBurger enabled auto-merge June 30, 2021 17:58
@JonnyBurger JonnyBurger merged commit d8cad39 into remotion-dev:main Jun 30, 2021
@frencojobs frencojobs deleted the feat/docs_twoslash branch July 1, 2021 06:05
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.

Add twoslash to docs
3 participants