-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
feat(v2): add typescript support for live code blocks (#2824) #3984
Conversation
@@ -11,13 +11,55 @@ import clsx from 'clsx'; | |||
|
|||
import styles from './styles.module.css'; | |||
|
|||
function Playground({children, theme, transformCode, ...props}) { |
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.
Removed transformCode
prop. Never coming down from parent components.
Also, got rid of ...props
because:
- this component is not written in Typescript, so it's hard to know what's inside ...props
- other keys that exist in
props
arelive
andmetastring
, which are not used here. Therefore excluding them.
const transformTs = React.useCallback((code) => { | ||
try { | ||
// eslint-disable-next-line global-require | ||
const {code: transformed} = require('@babel/standalone').transform(code, { |
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.
require
d inside here in the hope that it will dynamically import it only if there is a code block written in Typescript... but I'm not fully sure if this hack works.
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.
seems like it does work?
setTsTranspileError(e.message); | ||
} | ||
|
||
return '() => null;'; |
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.
in case if something goes wrong in the transpilation, render nothing. Theoretically it should never reach here.
Size Change: +9 B (0%) Total Size: 156 kB ℹ️ View Unchanged
|
✔️ Deploy preview for docusaurus-2 ready! 🔨 Explore the source changes: d72e21e 🔍 Inspect the deploy logs: https://app.netlify.com/sites/docusaurus-2/deploys/5ff086c001f60f00074dfcbd 😎 Browse the preview: https://deploy-preview-3984--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3984--docusaurus-2.netlify.app/classic/ |
hmm no idea why another irrelevant test failed after fixing one |
@@ -71,7 +71,7 @@ module.exports = { | |||
items: [{label: 'Twitter', to: 'https://twitter.com/docusaurus'}], | |||
}, | |||
], | |||
copyright: 'Copyright © 2020 Facebook Inc.', | |||
copyright: `Copyright © ${new Date().getFullYear()} Facebook Inc.`, |
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.
test broke it's a happy new year.
tests all passing again. |
Hi @9oelM Our bundle size monitoring system is a bit new and probably not setup to catch all possible bundle size increases, as it's highlighted in this PR. https://bundlephobia.com/result?p=@babel/standalone@7.12.12 Shipping babel standalone to the browser is a huge dependency We can clearly see it affecting the JS size download on pages using code blocks in your deploy preview. It even increases the pages that are not using TS code blocks, but any code blocks. Unfortunately, using Babel in client-side code this way is not really an option. Even with lazy-loading and dynamic imports, I'm not sure it's a good idea to increase all the page sizes of Hookstate using TS code blocks of 2mb of JS just for adding this feature. Think about doc readers having low-bandwidth internet connexions, that will be penalized with high React hydration times. So I'm going to close, but thanks for trying. |
thanks for taking a time to review this PR. Yeah. what you say totally makes sense... it's too big. Perhaps I will write a plugin personally. In the meantime, it'd be good to attach some bundle analyzer webpack plugin or related github action that gives visual representation of the bundle sizes of each file. Do you think it's possible to do this? |
@9oelM we have this build size bot in CI that is supposed to notify us now, but it's new and wasn't able to detect this size problem, unfortunately. This might just be due to missing pages/js files to monitor, as the JS is code splitted and the intro page does not contain any code block. If you want to help to improve the detection of perf/size regressions, help is welcome ;) Here's the initial issue to discuss all that: #3521 |
Motivation
Originally from #2824. For the live code blocks, we are using https://github.com/FormidableLabs/react-live, and it does not support Typescript, and they said they have absolutely no plans to support Typescript (See FormidableLabs/react-live#8)
However there is a need from the users (hopefully many... 😅) of Docusaurus who potentially would need Typescript support. Some projects written by our users (example: https://hookstate.js.org/ by @avkonst) are based on Typescript, so it would make a sense to support Typescript out of the box.
Concerns
I'm trying to introduce
@babel/standalone
to transpile Typescript and it works super nicely, but I am not sure how much bundle size it will add to the project. Will see results from the CI (update: seems like it's fine?)If this is too much of a burden to include Typescript support in the official plugin due to the bundle size, I'm more than happy to close this PR and release it as a personal one instead.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
To demonstrate that Typescript is supported to any potential users and to prove that it works, I simply added one code block that uses Typescript on https://deploy-preview-3984--docusaurus-2.netlify.app/classic/docs/next/markdown-features/code-blocks. I tested it and it should work, but if not, please tell me.
Will close #2824 if merged