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

feat: Added the ability to add custom themes/languages to Shiki #2518

Merged
merged 20 commits into from
Feb 7, 2022
Merged

feat: Added the ability to add custom themes/languages to Shiki #2518

merged 20 commits into from
Feb 7, 2022

Conversation

JuanM04
Copy link
Contributor

@JuanM04 JuanM04 commented Feb 1, 2022

Changes

  • Added wrap to Astro Markdown shiki config
  • Added a way to import custom themes and languages to Shiki (both for <Code /> and Markdown plugin)

Follow up to #2497

Testing

yarn test:match "Shiki|Code"

Docs

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2022

🦋 Changeset detected

Latest commit: 321a468

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
astro Patch
@astrojs/markdown-remark Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) docs pkg: example Related to an example package (scope) feat: markdown Related to Markdown (scope) labels Feb 1, 2022
@netlify
Copy link

netlify bot commented Feb 1, 2022

✔️ Deploy Preview for astro-docs-2 ready!

🔨 Explore the source changes: ffbe290

🔍 Inspect the deploy log: https://app.netlify.com/sites/astro-docs-2/deploys/61fc0fd6fa3f020008a82611

😎 Browse the preview: https://deploy-preview-2518--astro-docs-2.netlify.app/en/guides/markdown-content

@JuanM04
Copy link
Contributor Author

JuanM04 commented Feb 1, 2022

It seems like there are some timeout errors that aren't related to this PR. The difference between the latest two commits is in the indentation, and in the first commit windows fails and in the second macos fails

@matthewp
Copy link
Contributor

matthewp commented Feb 2, 2022

@JuanM04 the OSX test can be a little flakey, so I've rerun it.

Copy link
Contributor

@retronav retronav left a comment

Choose a reason for hiding this comment

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

The changesets have Shiki probably misspelled as "Shika"

@JuanM04
Copy link
Contributor Author

JuanM04 commented Feb 2, 2022

@obnoxiousnerd thanks!

@matthewp In the latest re-run that you've made, Windows threw this error:

 1) Astro Markdown Shiki
       Themes
         Custom theme
           "before all" hook for "Markdown file":
     Error: EPERM: operation not permitted, mkdir 'D:\a\astro\astro\packages\astro\test\fixtures\astro-markdown-shiki\themes\dist\astro'
      at Object.mkdirSync (node:fs:1334:3)
      at mkdirpath (D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:21723:8)
      at D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:21727:9
      at new Promise (<anonymous>)
      at writeFile (D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:21726:12)
      at writeOutputFile (D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:23708:25)
      at D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:23632:65
      at Array.map (<anonymous>)
      at handleGenerateWrite (D:\a\astro\astro\node_modules\rollup\dist\shared\rollup.js:23632:52)
      at runMicrotasks (<anonymous>)

Edit: I found the error. Windows fails if you put fixtures in other place that isn't fixtures/. For example, fixtures/markdown/themes needs to be fixtures/markdown-themes

Edit 2: no, it was a race condition.

@matthewp
Copy link
Contributor

matthewp commented Feb 2, 2022

Nice! Thanks @JuanM04

@@ -6,6 +6,9 @@
// helpful tooltips, and warnings if your exported object is invalid.
// You can disable this by removing "@ts-check" and `@type` comments below.
import astroRemark from '@astrojs/markdown-remark';
import fs from 'fs';

const riGrammar = JSON.parse(fs.readFileSync('./src/shiki/ri.tmLanguage.json'));
Copy link
Member

@FredKSchott FredKSchott Feb 3, 2022

Choose a reason for hiding this comment

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

this feels like a really advanced case, I feel like most users just want to change the theme, and not add a custom lang. I'd recommend removing all changes from this example.

If you wanted to mention something about this, you could maybe instead link to the docs with something like:

			{
				syntaxHighlight: 'shiki',
				shikiTheme: 'dracula',
+               // full documentation: ASTRO_DOCS_URL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a good appreciation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the changes. What do you think about them?

@@ -43,7 +50,10 @@ function repairShikiTheme(html: string): string {
}

const highlighter = await shiki.getHighlighter({ theme });
const _html = highlighter.codeToHtml(code, { lang });
if(typeof lang !== 'string') await highlighter.loadLanguage(lang)
Copy link
Member

Choose a reason for hiding this comment

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

is it okay if this runs on every codeblock render? If not, can you do a check that the language hasn't already been loaded?

Copy link
Contributor Author

@JuanM04 JuanM04 Feb 3, 2022

Choose a reason for hiding this comment

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

The language is loaded to the highlighter instance. Because the instance is being generated on every render, so does the loadLanguage function. I'll see if I can optimize it a bit more.

Edit: I've found a faster way to load only one language

const highlighter = await shiki.getHighlighter({
theme,
// Load custom lang if passed an object, otherwise load the default
langs: typeof lang !== 'string' ? [lang] : undefined
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@@ -12,6 +44,13 @@ const remarkShiki = async (theme: shiki.Theme) => {
html = html.replace('<pre class="shiki"', '<pre class="astro-code"');
// Replace "shiki" css variable naming with "astro".
html = html.replace(/style="(background-)?color: var\(--shiki-/g, 'style="$1color: var(--astro-code-');
// Handle code wrapping
// if wrap=null, do nothing.
if (wrap === false) {
Copy link
Member

Choose a reason for hiding this comment

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

I would really like to not mess with the default style by default, unless we consider them broken. I would expect shiki's default output without changes to be fine / not overflow. Is that not the case / do we consider the default shiki output broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I don't know. I've just mimicked <Code /> behavior

Copy link
Member

Choose a reason for hiding this comment

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

Right, but we don't currently add overflow-x in <Code> today, right? That's what I'm wondering about, since I hadn't heard this complaint / that a fix for this was needed and I generally like us to match default behaviors when we can.

Copy link
Contributor Author

@JuanM04 JuanM04 Feb 3, 2022

Choose a reason for hiding this comment

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

Right, but we don't currently add overflow-x in <Code> today, right?

Actually, yes. I double checked with a brand new app.

That's what I'm wondering about, since I hadn't heard this complaint

I'm as confused as you. So, I did a little research (thanks Linus for making git blame) and I found this comment: #1208 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

lol I did this??? Welp, nevermind then 🤦 I'm suprised that I didn't file this as a bug of Shiki then, maybe its trapped in one of my other issues on that repo.

Either way, thanks for digging! this LGTM then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol hahaha

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM

@FredKSchott
Copy link
Member

uh oh, tests failing!

@JuanM04
Copy link
Contributor Author

JuanM04 commented Feb 4, 2022

@FredKSchott I've been fighting Windows tests for so long, and I don't know why some times they fail and some times they pass

@FredKSchott
Copy link
Member

hmm, race condition? Did a recent commit introduce or has this always been happening?

I'm about to spin up my own PR, will verify if its an issue on main

@JuanM04
Copy link
Contributor Author

JuanM04 commented Feb 4, 2022

@FredKSchott I think it's because I do multiple builds with the same folder. But this isn't the only test that does that. Is there a way to force mocha/chai to run a suit synchronously?

Also, it never happened on Linux nor Mac (although Mac sometimes throw an error in "before all" hook in "{root}", but I've seen it in other PRs)

@JuanM04
Copy link
Contributor Author

JuanM04 commented Feb 5, 2022

In fact, was a race problem. @FredKSchott, could you re-run the test another time just to be sure?

Btw, I've read somewhere that Vitest lets you control concurrency in a test-by-test basis

@jonathantneal
Copy link
Contributor

Hey @JuanM04. Like I’m mentioning in your other PR, we recently migrated our docs site over to https://github.com/withastro/docs

I have another PR to remove our old version of the docs from this monorepo, #2517.

I can make sure your work gets added into that repository. Would you be interested in making that PR? You don’t have to, but I would like you to get the commit credit. I can also attribute the improvement to you, but it would not be the same as a signed commit from your machine.

@FredKSchott
Copy link
Member

LGTM!

@FredKSchott FredKSchott merged commit 2bc9154 into withastro:main Feb 7, 2022
@github-actions github-actions bot mentioned this pull request Feb 7, 2022
@JuanM04 JuanM04 deleted the shiki-config branch February 7, 2022 16:34
@github-actions github-actions bot mentioned this pull request Feb 18, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
…astro#2518)

* Replaced `shikiTheme` with `shikiConfig`

* Code.astro now accepts custom themes/langs

* Updated docs

* Updated tests

* Fixed language loading

* Added customization examples

* Updated documentation

* Added more tests

* Changelogs

* Changed some spaces to tabs

* Fixed typo in changesets

* Moved tests fixtures

* Rolled back changes to with-markdown-shiki

* Removed lang example in docs

* Optimized Code component

* Try to fix windows errors

* Try to see if this new tests work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants