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

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

Closed
3 of 4 tasks
xsjcTony opened this issue Aug 6, 2024 · 6 comments
Closed
3 of 4 tasks

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

xsjcTony opened this issue Aug 6, 2024 · 6 comments

Comments

@xsjcTony
Copy link

xsjcTony commented Aug 6, 2024

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

  • If this feature request is accepted, I am willing to submit a PR to fix this issue
@uxiew
Copy link
Contributor

uxiew commented Aug 6, 2024

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
Copy link
Author

xsjcTony commented Aug 8, 2024

@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
Copy link
Contributor

uxiew commented Aug 8, 2024

@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
Copy link
Author

xsjcTony commented Aug 8, 2024

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
Copy link
Author

@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
Copy link
Member

antfu commented Aug 12, 2024

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:

  • Fundamentally change how tokenized work to have a single pipeline (we might need to fork vscode-textmate)
  • We create a "Proxy" state, that mapping to multiple themes. I am not sure how feasible it would be.

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

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

No branches or pull requests

3 participants