[lexical-code][lexical-code-shiki][lexical-markdown][lexical-playground][lexical-devtools] Feature: Experimental Shiki support for code highlighting#7662
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
etrepum
left a comment
There was a problem hiding this comment.
some initial comments, did a quick look but not very thorough
| if (languageInfo) { | ||
| // in case we arrive here concurrently (not yet loaded language is loaded twice) | ||
| // shiki's synchronous checks make sure to load it only once | ||
| await shiki.loadLanguage(languageInfo.import()); |
There was a problem hiding this comment.
using an await will force this to get deferred to the next microtask and cause some extra rendering, there should really be something that will use promises only when loading is in flight. similar to how promises work with react suspense.
There was a problem hiding this comment.
I am not sure I get the full extent of your comment. I removed the await and replaced it with
shiki.loadLanguage(languageInfo.import()).then(..)
or do you mean languageInfo.import().then((module)=>shiki.loadLanguage(module); ..) ?
What I found about react suspense is
React catches that promise, pauses rendering, and shows the fallback UI you provided to . When the promise resolves, React tries rendering again. This is possible because in JavaScript, you can synchronously stop a function by throwing. React leverages this to "pause" rendering until the data is ready.
can you clarify what you mean by "there should really be something that will use promises only when loading is in flight. similar to how promises work with react suspense." ?
currently, CodeNodeTransform
- fire-and-forget the loadAsset(language or theme) when they're not loaded + bails out if either needed loading. After loading, codeNode is marked as dirty
I could also not bailout after observing that loading is needed and render the code with lang "plain" + theme "none" which are always loaded
| editor.update(() => { | ||
| const codeNode = $getNodeByKey(codeNodeKey); | ||
| if ($isCodeNode(codeNode)) { | ||
| codeNode.setTheme(theme); |
There was a problem hiding this comment.
There should be some way to do this only once when the theme transitions from a not loaded to a loaded state to avoid running the transforms more times than necessary
| codeNode.setTheme(theme); | |
| codeNode.markDirty(); |
There was a problem hiding this comment.
yes, codeNode.markDirty(); is sufficient indeed. I am not sure if your comment refers to an additional optimization that could be done to lower the number of transform applications between the time when we receive the setTheme & the time we know the theme has been loaded.
I am not sure I am fully aware of what is the overhead difference between codeNode.setTheme(theme); and codeNode.markDirty();.
|
@etrepum thanks for the comments I will look into them. For now I am blocked on the build process in scripts/build.js that seems to not like my dynamic imports. I will try to find time to look into it but am not familiar with this script and the extent to which I can change it. i think the problem arises when the build script tries to bundle https://github.com/shikijs/shiki/blob/main/packages/shiki/src/langs-bundle-full.ts and https://github.com/shikijs/shiki/blob/main/packages/shiki/src/themes.ts which have all the dynamic imports for all shiki languages |
|
Just do it without the async imports for now and that can be handled separately. |
etrepum
left a comment
There was a problem hiding this comment.
I think this is in a good state! I do like the idea of a line-based approach for shiki, which will certainly help with other UI affordances like making the gutters correct, but I think that can be handled as a follow-up. We might want to do some additional doc changes to note that this package is experimental and we expect to change the internals for that purpose.
Any thoughts on adding this @lexical/code-shiki engine? /cc @ivailop7 @zurfyx
The big highlights for me are:
- Shiki is a maintained dependency
- It leverages a standard TextMate grammar format for its syntax (same as vscode)
- Lazy loading of language and theme files without too many hacks and without risk of breaking prism
Some follow-up items to consider:
- Extracting prism to its own optional package (breaking change)
- Build time preloading of syntax and theme modules
- Restructuring shiki highlighted code blocks to be line-oriented (and having a gutter implementation for it)
|
One thing that would be helpful while this gets some feedback from other maintainers would be to reformat the PR title/description to match the pull request template It would also be good to note the (minor) breaking change prominently, typically with something like this: |
etrepum
left a comment
There was a problem hiding this comment.
Made some minor changes, primarily to preserve constructor signature compatibility with the existing CodeNode
One other thing we've been missing is just pure |
This PR is a big step in that direction, it decoupled CodeNode from Prism and any highlighting dependencies (which is why the timing of setting the highlight language attribute changed). The remaining step would primarily be to break compatibility by moving the Prism code to its own package. |
|
You can test out the playground with shiki enabled here: You can see it without any highlighting plugin enabled here: |
…nd][lexical-devtools] Feature: Experimental Shiki support for code highlighting (#7662) Co-authored-by: Bob Ippolito <bob@redivi.com>
Awesome :) Like so, lexical-markdown can import triple-backticks and still render it maybe even without dependencies on Prism/Shiki (if highlighting is not needed, just some display of block of code) or even on lexical-code |
|
CodeNode will remain in lexical-code, eventually prism support will be moved to an optional package. |
Fixes #13386 Below I write a clarification to copy and paste into the release note, based on our latest upgrade of Lexical [in v3.29.0](https://github.com/payloadcms/payload/releases/tag/v3.29.0). ## Important This release upgrades the lexical dependency from 0.28.0 to 0.34.0. If you installed lexical manually, update it to 0.34.0. Installing lexical manually is not recommended, as it may break between updates, and our re-exported versions should be used. See the [yellow banner box](https://payloadcms.com/docs/rich-text/custom-features) for details. If you still encounter richtext-lexical errors, do the following, in this order: - Delete node_modules - Delete your lockfile (e.g. pnpm-lock.json) - Reinstall your dependencies (e.g. pnpm install) ### Lexical Breaking Changes The following Lexical releases describe breaking changes. We recommend reading them if you're using Lexical APIs directly (`@payloadcms/richtext-lexical/lexical/*`). - [v.0.33.0](https://github.com/facebook/lexical/releases/tag/v0.33.0) - [v.0.30.0](https://github.com/facebook/lexical/releases/tag/v0.30.0) - [v.0.29.0](https://github.com/facebook/lexical/releases/tag/v0.29.0) ___ TODO: - [x] facebook/lexical#7719 - [x] facebook/lexical#7362 - [x] facebook/lexical#7707 - [x] facebook/lexical#7388 - [x] facebook/lexical#7357 - [x] facebook/lexical#7352 - [x] facebook/lexical#7472 - [x] facebook/lexical#7556 - [x] facebook/lexical#7417 - [x] facebook/lexical#1036 - [x] facebook/lexical#7509 - [x] facebook/lexical#7693 - [x] facebook/lexical#7408 - [x] facebook/lexical#7450 - [x] facebook/lexical#7415 - [x] facebook/lexical#7368 - [x] facebook/lexical#7372 - [x] facebook/lexical#7572 - [x] facebook/lexical#7558 - [x] facebook/lexical#7613 - [x] facebook/lexical#7405 - [x] facebook/lexical#7420 - [x] facebook/lexical#7662 --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1211202581885926
This PR creates a new
lexical-code-shikipackage that adds support for the Shiki highlighting engine.By default, [lexical-playground] still uses the Prism highlighting engine in [lexical-code]. New setting options in th playground make it possible to enable Shiki highlighting.
[lexical-code-shiki] implements dynamic support for all languages and themes supported by Shiki.
Prism support can be considered backward compatible except for one minor aspect enabling CodeBlocks to have dynamic loading of languages (see Breaking Changes below). This affects a small number of tests in [lexical-code] and [lexical-markdown].
The support is considered experimental in the sense that more work is expected on it before shiki support should be considered backwards compatible. Some things are still subject to change such as:
Breaking Changes
CodeNow now applies the
data-highlight-languageDOM attribute after highlighting, rather than immediately. Thedata-languageattribute behavior is unchanged. This post-highlighting setting also applies togetIsSyntaxHighlightSupported()and its underlying__isSyntaxHighlightSupportedproperty. Previously this would have been set eagerly because all known Prism languages were known and loaded at build time, now it represents whether the syntax highlighting has occurred or not.Tests
All tests related to Prism have been left unmodified except those mentioned related to the Breaking changes.
Tests have been added to Shiki to control that all the Tab/Indent/Outdent cases pass.
Manual tests have been applied, but other Shiki tests that imply the tokenization of the code have been postponed until the restructuring of shiki highlighted code blocks to be line-oriented, as all the expected html snippets will need updating. These Shiki tests will be written by taking all the Prism tests as a starting point.
The Prism tests themselves will be kept as-is.