feat(shiki): add twoslash support #8132
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8132 +/- ##
==========================================
- Coverage 76.58% 76.42% -0.17%
==========================================
Files 115 115
Lines 9602 9629 +27
Branches 322 316 -6
==========================================
+ Hits 7354 7359 +5
- Misses 2247 2269 +22
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
Hype 😁 ! |
|
Locally: |
avivkeller
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds TypeScript TwoSlash support to the syntax highlighting system, enabling interactive TypeScript error displays and type information tooltips in code blocks.
Key Changes:
- Integrated TwoSlash transformer for TypeScript/JavaScript code highlighting
- Refactored metadata parsing system from string-based to object-based approach
- Added CSS styling for TwoSlash popup containers and error displays
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rehype-shiki/src/index.mjs | Added TwoSlash transformer import and configuration |
| packages/rehype-shiki/src/highlighter.mjs | Updated functions to accept metadata parameter and pass transformers |
| packages/rehype-shiki/src/plugin.mjs | Replaced string-based metadata parsing with comprehensive object-based system |
| packages/rehype-shiki/src/twoslash.css | Added CSS styling for TwoSlash popup containers |
| packages/rehype-shiki/package.json | Added TwoSlash dependency and exported CSS file |
| packages/ui-components/src/Common/BaseCodeBox/index.module.css | Added CSS transforms for proper positioning |
| apps/site/next.config.mjs | Configured external packages and file tracing for TwoSlash |
| apps/site/package.json | Added twoslash dependency |
| apps/site/styles/index.css | Imported TwoSlash CSS styles |
| apps/site/pages/en/learn/typescript/transpile.md | Updated TypeScript example to use TwoSlash error annotations |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Is this all server side? I'm asking because given the TS port to Go, I'm not sure what the deal will be for shiki on the web; probably having to include a large Go binary in Wasm to work with all of the fancy stuff if not prerendered... |
It's not supposed to be passed to the client at all, so we should definitely investigate that |
Fixed in the latest commit, which introduced some styling issues (regarding wrapping). |
Is that even possible to fix? iiuc CSS doesn't allow us to set this element to width + overflow? |
|
The size increase comes from the additional HTML needed for these tooltips |
|
I don't like that it's all chunked on the main page, this would be better as an imported script, but that would need to be changed as part of our Markdown to JSX |
Sorry @avivkeller, I only now saw your message 🙇 Tomorrow I'll have a closer look 🙇 , however by quickly skimming though your changes I didn't spot anything that could be problematic, on the contrary, it seems like you are conditionally using the wasm implementation only if Do you have any suspicion on what might be causing the Cloudflare incompatibility? 🙂 |
|
@avivkeller I'm looking into the issue, for now what I've seen is that this problematic but I thought that in our setup shiki would not use wasm? 🤔 |
|
Our Shiki setup doesn't use WASM when |
Yes, that's exactly what I saw/thought, but definitely shiki seems to be trying to instantiate wasm anyways here 😕 (Also via the debugger I can indeed see that
In the Minification
To make the code a bit more decipherable I've disable minification in the next config: |
|
FYI @dario-piotrowicz, still not working |
|
Bump @dario-piotrowicz |
avivkeller
force-pushed
the
twoslash
branch
from
dcabc2b to
9711c95
Compare
|
@nodejs/nodejs-website @nodejs/web-infra for reviews. Currently, this is disabled on CF, but there's a TODO to enable it. |
|
|
||
| These type definitions allow TypeScript to understand Node.js APIs and provide proper type checking and autocompletion when you use functions like `fs.readFile` or `http.createServer`. For example: | ||
|
|
||
| ```js |
There was a problem hiding this comment.
This is supper weird. I see the idea of showing auto completion.
But it's strange
When I was looking at this, I was expecting an interactive thing.
There was a problem hiding this comment.
How so? I can change it if it's not good
There was a problem hiding this comment.
- (big) solution having "editor" that user can use to learn such as a complet learning platform. IMO not good idea.
- just don't use this feature
There was a problem hiding this comment.
I think it makes sense, as we are discussing auto-complete, cc @nodejs/nodejs-website @nodejs/typescript for opinions
|
I'll get to Augustin's review today, @nodejs/web any other comments / concerns |
Fixes #7932
Introduces
@shikijs/twoslashsupport in@node-core/rehype-shiki. Sincetwoslashdepends on TypeScript declaration files, it cannot be bundled and must instead be listed underserverExternalPackages.Because TypeScript declarations aren’t imported like regular files (they’re loaded through a TSVFS), they need to be explicitly included in the output bundle.
Because Cloudflare doesn't support WASM or Top-Level awaits, rehype-shiki has been rewritten to not rely on them.