-
-
Notifications
You must be signed in to change notification settings - Fork 518
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: use slim shiki build and pre-compiled languages #1519
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
This is great!
If I understand it correctly; this mostly makes sure unused languages that are not in our "defaultSupportedLanguages" don't appear in the app output. However, languages that are, of course still appear in the app output. correct?
The ideal would be for the user to specifically pick which languages and themes for the code block feature, rather than us providing any defaults for this.
Perhaps we can have defaults built into a separate package? To make it easier for those already using us?
I think it's worthwhile to tackle the issue completely and invest a bit more to also address this.
- I think it's ok to make this a breaking changes as long as it's clearly documented
- if it's not too much overhead (in code / docs / maintenance) I think having some sensible defaults easily available makes sense indeed
- we should decide whether by default we don't want any syntax highlighting at all, or only support 1-3 langs
- while we're at it, maybe we should also make it easy for consumers to load other themes
Great news. My biased opinion would be to not add any and let the consumers decide to include one or multiple somehow. I would see the code block as a more 'advanced functionality' that will be used by consumers that should be fine with doing some config. If you throw in an example of that on your site, the first round of beers in de Biergarten are on me 🍻 😸. |
Alright, I've exposed the codeBlock configuration on the editor instance, and so this would let you configure the languages & themes that are supported by your app. By default, no languages or themes are bundled, to configure syntax highlighting, you can install the new package If you want more control than this, I'll write a guide on how easy it will be to integrate your list of languages, using the shiki-codegen tool. This isn't completely ready yet, as I'll need to:
|
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.
awesome!! only some comments / qs about naming and docs
<div class="bn-block-group" data-node-type="blockGroup"><div class="bn-block-outer" data-node-type="blockOuter" data-id="1"><div class="bn-block" data-node-type="blockContainer" data-id="1"><div class="bn-block-content" data-content-type="codeBlock"><pre><code class="bn-inline-content">console.log('Hello, world!');</code></pre></div></div></div></div> |
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.
is it easy / needed to add tests whether the exporting / importing to / from md / html still works for custom languages?
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.
I think we have some existing tests for this: https://github.com/TypeCellOS/BlockNote/blob/013818d40a2fedaf192ddc7021a8457b2eac37bc/packages/core/src/api/exporters/html/__snapshots__/codeBlock/python/internal.html#L0-L1
Just didn't need to modify it because it was only the default language that changed (it went from being javascript
by default to text
)
/** | ||
* Options for code blocks. | ||
*/ | ||
codeBlock?: CodeBlockOptions; |
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.
minor thought; is it confusing that the term codeBlock
is used for all of:
- the blocknote block (defined in core)
- the option for the theme / highlighting (defined here)
- the package for advanced theme / highlighting
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.
codeBlock
(this option) makes sense as it is the bundle of options for configuring the code-block block type (like tables)
@blocknote/code-block
(the package) I think could maybe be renamed to @blocknote/defaults
with an export of codeBlock
which could make sense if we decide to add more things like this. Or, similar.
@@ -0,0 +1,33 @@ | |||
# Code Block Highlighting |
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.
The pattern we currently use is that we have:
- max 1 or 2 paragraphs in the example as description
- an optional "try it out" line to explain how the user can see the functionality in the demo
- links to relevant docs
The idea is that the examples can really be a larger set of examples to click through and play with without too much reading - and that all documentation should be in /docs.
Could we change this? or lmk if you like a different setup
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.
Totally makes sense. Updated
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.
Looks good! Only a few suggestions
}, | ||
initialContent: [ | ||
{ | ||
id: "fc832df4-bd15-49d2-8d64-140c27f29692", |
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.
Same thing as previous comment r.e. using PartialBlock[]
instead of Block[]
```javascript | ||
console.log('Hello, world!'); | ||
``` | ||
console.log('Hello, world!'); |
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.
Not sure we can really do much about it either (since the HTML -> MD conversion is not handled by us), but this seems to be missing the code block formatting.
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.
Yea, I spent like 30 minutes looking into it. It does not want to properly convert it if it does not have a language specified
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.
Would it make sense to render a language-text
attribute to the HTML to fix this?
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.
I will just output language-text, I don't see the harm in it
This example proves it should work fine
This significantly cuts down on the bundle size of the package by leveraging shiki's new slim builds and precompiled languages
Fixes #1487 #1213
This could probably be reduced further by completely separating the default languages, but for this would require a breaking change to the API for code blocks to properly work.
The ideal would be for the user to specifically pick which languages and themes for the code block feature, rather than us providing any defaults for this.
Perhaps we can have defaults built into a separate package? To make it easier for those already using us?
Either way, this would require some thought, while the current change would still be compatible with the current API