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

Remove the default loading of nord theme when getHighlighter #473

Closed
kricsleo opened this issue May 24, 2023 · 3 comments · Fixed by #557
Closed

Remove the default loading of nord theme when getHighlighter #473

kricsleo opened this issue May 24, 2023 · 3 comments · Fixed by #557

Comments

@kricsleo
Copy link

kricsleo commented May 24, 2023

If theme or themes is empty when using getHighlighter, then the nord theme will be loaded by default. This results in an unnecessary theme load for situations where the theme needs to be loaded dynamically (or an unnecessary theme resource request in the browser environment).
For example:

// Here there will be an unwanted `nord` theme load.
// Because later the required theme will be loaded dynamically depending on the situation.
const highlighter = await shiki.getHighlighter({})

export async function highlight(code, lang, theme) {
  // Here the real theme will be loaded, e.g. `github-dark`
  await highlighter.loadTheme(theme)
  return highlighter.codeToHtml(code, { lang, theme })
}

Can we remove the default theme logic here? Or a way to disable it in the above situations. Thanks :)

if (!_themes.length) {
_themes = ['nord']
}

(Willing to contribute a PR for this)

@orta
Copy link
Contributor

orta commented May 24, 2023

Give that this would be a massive breaking change, it probably shouldn't happen this way unless it ends up in the 1.0 changes in #424 and is very well documented

Perhaps passing null could be accepted as the theme and have this behavior instead

@kricsleo
Copy link
Author

I agree that removing the logic here directly may produce incompatible changes, although the documentation doesn't explicitly mention that the default theme will be loaded, it may be the default behavior when the user relies on not passing theme.

If, as you say, we remove the loading of the default theme when the user is shown using theme: null or themes: null, would this be acceptable as a patch version of v0.14?

@orta
Copy link
Contributor

orta commented May 25, 2023

Sure, allowing specifically for null is something I'd be OK with merging (as long as that's documented in the jsdoc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants