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: use markdown-it-async, remove synckit #4507

Merged
merged 11 commits into from
Jan 25, 2025
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented Jan 22, 2025

Description

Changed markdown-it to markdown-it-async, to eliminate the sync limitation. This makes Shiki able to load languages on-demand without synckit, which would block the thread. In the long run, make markdown-it work with async would also unlocks more potential features.

This slightly changes https://vitepress.dev/reference/default-theme-search#custom-content-renderer, which may require migration on user land. Considering it might only affect a tiny fiction of users for this advanced feature, I am not sure if VitePress considers this a breaking change. If we want to warn users for migrate, we can also enable this https://github.com/antfu/markdown-it-async#opt-in-warning

The downside of this approach is that it will breaks the Twoslash integration again - which wasn't the expected usage anyway - I have updated Shiki's docs to ask users to explicit provide the language names: https://shiki.style/packages/vitepress#setup. We should either ask users to add those languages, or always initiate them by default (which would sacrifices a bit of performance). In the meantime, I am also working on Shiki's async transformers support, which might make Twoslash integration more clean and efficient.

Linked Issues

Additional Context


Tip

The author of this PR can publish a preview release by commenting /publish below.

@antfu antfu marked this pull request as draft January 22, 2025 08:29
@userquin
Copy link
Member

When building the client types, it is resolving markdown-it to cjs and it doesn't export the types (only default), check the d.ts and the d.mts files.

You can add tsconfig: 'tsconfig.json' to the client types dts plugin:

    dts({
      respectExternal: true,
      tsconfig: 'tsconfig.json',
    }),

image

With this change the build is fine, but we get 3 warnings:

image

/cc @brc-dd maybe you can comment here ;)

@userquin
Copy link
Member

userquin commented Jan 22, 2025

The CI is fine, check the build on Windows, the warnings are there but on Linux no.

NOTE: removing repectExternal from client types dts plugin the warnings are gone (Windows)

@antfu antfu marked this pull request as ready for review January 22, 2025 20:56
@antfu antfu changed the title perf: use markdown-it-async, remove synckit feat: use markdown-it-async, remove synckit, make markdown.config and markdown.preConfig accept async function Jan 22, 2025
@antfu antfu changed the title feat: use markdown-it-async, remove synckit, make markdown.config and markdown.preConfig accept async function feat: use markdown-it-async, remove synckit Jan 22, 2025
@antfu antfu force-pushed the perf/markdown-it-async branch from 6db02c7 to 758180d Compare January 22, 2025 21:08
@brc-dd brc-dd marked this pull request as draft January 23, 2025 18:17
@brc-dd brc-dd marked this pull request as ready for review January 23, 2025 19:24
Copy link
Member

@brc-dd brc-dd left a comment

Choose a reason for hiding this comment

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

except for that twoslash thing, LGTM

let's check on iconify website once too. they have many mdit plugins

I'm curious though, as for most users there won't be much perf gain unless they are using too many languages. On vitepress docs: 👇

# before

pnpm docs:build:only   21.77s  user 4.45s system 118% cpu 22.102 total
max memory:                1463952 KB

# after

pnpm docs:build:only   22.08s  user 4.70s system 118% cpu 22.603 total
max memory:                1334800 KB

haven't checked the perf difference on dev though 👀

@brc-dd
Copy link
Member

brc-dd commented Jan 23, 2025

/publish

Copy link

pkg-pr-new bot commented Jan 23, 2025

npm i https://pkg.pr.new/vitepress@4507

commit: 0a40326

@antfu
Copy link
Member Author

antfu commented Jan 23, 2025

Yeah, I don't expect this to have a huge performance gain either, as the previous synckit workaround is already doing it on-demand (even tho I'd consider that approach being a bit hacky). I see it's more valuable to have markdown-it being able to be async, as it opens a lot more possibility. For example, in the future we might be able to off load the entire Shiki process to a worker for non-blocking and potentially parallelization. Shiki focused a lot on sync operations that hugely because integrations like markdown-it requires sync highlighting, and I am thinking to explore more potentials if we no logner need to be limited by that.

@brc-dd brc-dd merged commit 8062235 into main Jan 25, 2025
11 checks passed
@brc-dd brc-dd deleted the perf/markdown-it-async branch January 25, 2025 10:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants