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(plugin-shiki): migrate to esm-based shikiji (close #12) #13

Merged
merged 7 commits into from
Dec 28, 2023

Conversation

nruffing
Copy link
Contributor

@nruffing nruffing commented Dec 24, 2023

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Provide a description in this PR that addresses what the PR is solving. If this PR is going to solve an existing issue, please reference the issue (e.g. close #123).

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Other

Description

Shikiji is a pure esm rewrite of shiki, with some improvements like be able to set a separate light and dark theme.

Breaking changes between shiki and shikiji are documented here.

The changes in this PR do not use the compatibility build since it seems like now is a good time to make some breaking changes to gain the full benefits of not using the compatibility build.

I was able to adjust the options interface for plugin-shiki to still support supplying a single theme with the theme property. There are then new lightTheme and darkTheme properties. When creating the options to pass to codeToHtml the plugin will use light/darkTheme and fallback to theme and then 'nord' being the existing default value.

The other big change is that previously if no languages were specified they were all loaded by shiki. Shikiji doesn't load any languages in that case and you now have to specify languages. However it does print a pretty decent error message at build time if you are missing a language you referenced.

image

I also made changes to the docs site to use this version of the plugin and leverage the dark and light modes. The diff for those changes can be seen here and I can create a PR for that once a new build of this gets out.

I also published that version of the docs site here.

If we want to continue with this approach does it make sense to use this by default in theme-default now?

Screenshots

Before

image

image

After

image

image

closes #12

@meteorlxy
Copy link
Member

👍 Good work @nruffing

I'm thinking about either:

  • Adding e2e tests for ecosystem repo
  • Putting ecosystem docs inside ecosystem repo (remove from vuepress/docs repo)

so that we can test ecosystem changes more easily. Do you have any ideas about that?

@nruffing
Copy link
Contributor Author

In this case it wasn't too bad to set up the docs site locally with a pnpm link to the local package to test the changes. I do agree though that they may not always be easy to do and it's not self contained in this repo.

I do think it was a little confusing at first which parts of the current docs are for vuepress and which parts are for the default theme but I don't know if separating them would help with that or not.

I kind of like the e2e tests idea. It would force testing of the default theme and plugins outside the context of the vuepress docs site.

@meteorlxy
Copy link
Member

meteorlxy commented Dec 26, 2023

Yep, I also think e2e should be necessary.

My thought about putting ecosystem docs inside this repo is that, the core packages and ecosystem packages would be released separately in the future, then the version number in the navbar will likely be inconsistent. In that case, making the ecosystem docs a sub site of the official docs (e.g. set base to /ecosystem/, or deploy to some sub domain ) might be a better choice.

@meteorlxy
Copy link
Member

@nruffing I just made some updates to this PR. Let me know if you have any thoughts about my changes.

@nruffing
Copy link
Contributor Author

@meteorlxy Changes look good to me.

@meteorlxy meteorlxy merged commit df11c04 into vuepress:main Dec 28, 2023
8 checks passed
@nruffing nruffing deleted the shikiji branch December 28, 2023 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] plugin-shiki migrate to use shikiji
2 participants