shikijs / shiki

A beautiful yet powerful syntax highlighter
http://shiki.style/
MIT License
10.1k stars 370 forks source link

Make grammar state working with `themes` instead of just `theme` #734

Closed xsjcTony closed 2 weeks ago

xsjcTony commented 2 months ago

Clear and concise description of the problem

Currently it's not allowing to use themes when passing grammar state into codeToHtml

Either way it's not going to work no matter I specify theme in .getLastGrammarState or not. Seems it defaults to the first loaded theme. See the below example.

Also, seems the grammar state is not in the documentation, so I have to follow the guide in the PR and from TS types.

e.g.

const highlighter = await getSingletonHighlighterCore({
  themes: [
    import('shiki/themes/vitesse-light.mjs'),
    import('shiki/themes/one-dark-pro.mjs'),
  ],
  langs: [
    import('shiki/langs/typescript.mjs'),
  ],
  loadWasm: getWasmInstance,
});

export const stateTSTypeAnnotation = highlighter
  .getLastGrammarState('let a:', { lang: 'ts' });
// or with
  .getLastGrammarState('let a:', { lang: 'ts', theme: 'one-dark-pro' });
function renderType(type: string): string {
  return codeToHtml(
    type,
    {
      lang: 'ts',
      // theme: 'one-dark-pro', // only this is going to work if I specify `theme` in `.getLastGrammarState()` above
      themes: {
        light: 'vitesse-light',
        dark: 'one-dark-pro',
      },
      grammarState: stateTSTypeAnnotation,
      structure: 'inline',
    },
  );
}

image image

Suggested solution

allow themes in .getLastGrammarState(), or simply remove theme from the function. I've no idea how it's working under the hood, but it's a bit weird to me since we are passing the grammar state to the render function, where theme or themes has to be provided, then why do we still need to pass it to grammar state?

Alternative

No response

Additional context

No response

Validations

Contributes

uxiew commented 2 months ago

Im dealing with the same problem right now too.

I read the code. I think if (options.grammarState.theme !== themeName) { in code-to-tokens-base file but codeToTokensWithThemes use map to iterate over all the themes cause the Problem. maybe, should only determine if the theme provided by the user is in themes options, or just remove this line.

getLastGrammarState function need the theme parameter,for the user change the theme

Finally, I think maybe should export this GrammarState getLastGrammarState, so the grammarState options could controlled by user.

xsjcTony commented 2 months ago

@uxiew I don't get it.

getLastGrammarState function need the theme parameter,for the user change the theme

Why is that? the grammar state is just a state that will be passed into functions like codeToHtml, which eventually will require theme to be explicitly passed. I don't see why it has to be aware of the theme context

Or alternatively, the quick solution could be if theme / themes is provided, then ignore the theme passed to the grammar state.

uxiew commented 2 months ago

@xsjcTony yeah, it doesn't seem to be necessary at present like you said. GrammarState needs hold the current theme and lang, but it can acquire the theme、lang from those functions like codeToHtml. the code design from #715

xsjcTony commented 2 months ago

True. Anyway it makes no sense to complain about the theme mismatch, where the theme/themes from high-level functions like codeToHtml() should definitely take precedence.

I currently don't have time capacity to look into the code and provide a PR, but that's really a huge headache, which means I can't really use GrammarState in an app with two themes, otherwise it's not looking good at all🤦‍♀️

xsjcTony commented 2 months ago

@antfu Sorry for pining straight away, first of all, I really appreciate your hard work on the grammar state as it really helps a lot to annotate types independently.

Since you are the one who implemented the grammar state feature, can you please post some comments/thoughts about why it requires theme context and why it only accepts a single theme instead of themes for toggling between dark/light mode?

If you can share some thoughts there, maybe some other folks can have a look into it, as currently it really struggles with theme toggling.

Thanks 💖

antfu commented 2 months ago

The problem is that both theme and grammar would affect how tokens been generated. The current multiple themes approach is done by taking two passes with different themes, and then "sync" the tokens split points. Which means in this process, there are actually two or multiple pipeline of "grammar state" been generated. Supposely that we only return the state of the first themes.

So this problem is rather tricky to handle, we either:

Either way, I don't think I have enough bandwidth or incentive to work on that. I would count on the community's contributions.