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: replace prism with starry night #67

Closed
wants to merge 5 commits into from

Conversation

lino-levan
Copy link
Contributor

Closes #60 and #56.

@lino-levan
Copy link
Contributor Author

@kt3k PTAL. One small note, this PR currently uses the all field (which supports all languages), but should we instead provide config for adding languages? The API would have to be different from what he had in the past (somewhat obviously since we're not using prisma anymore).

@kt3k
Copy link
Member

kt3k commented May 12, 2023

I'm not convinced starry-night is stable/reliable enough. It's very new (1 year old) and it has very much smaller number of downloads in npm compared to prismjs. https://npmtrends.com/@wooorm/starry-night-vs-prismjs Also it doesn't seem solving any specific problem currently we have. I feel we don't have enough motivation to switch to starry-night at this point

@lino-levan
Copy link
Contributor Author

@kt3k it does solve a few problems we currently have (but one could argue that they aren't big enough to worry about):

Additionally, as @kidonng correctly points out, it's by the same author as remark (which is the library we will hopefully be moving over to). Unless we want to hack prism support into remark, we will be switching to starry night in the future regardless of our decision on this PR.

I'm not married to the idea of using starry night, and I would be more than willing to accept that Prism is all that is in the scope of the project, but that's more of a discussion of #57.

@kt3k
Copy link
Member

kt3k commented May 12, 2023

The same author also maintains 2 other syntax highlighters: refractor and lowlight

The author says starry-night is slower than other 2 (wooorm/starry-night#20 (comment)). That sounds concerning to me because we highlight source code on the fly

@lino-levan
Copy link
Contributor Author

Would you prefer if we switched to one of the other two? I'd have no qualms with it.

@lino-levan
Copy link
Contributor Author

I think we should have performance benchmarks anyways, I'll bench this over the current implementation in main. We should not merge regardless until we see what that looks like.

@lino-levan
Copy link
Contributor Author

Fair benchmark is being blocked by: esm-dev/esm.sh#627

but from my initial testing it seems to be ~10x slower than Prism which is slower but also not unreasonably slow. We have a few options here:

  • Go with starry-night despite this slowdown (because most of the time spent generating the HTML is spent in the markdown phase anyways so highlighting isn't that big of a deal)
  • Say "it's not worth it" and stick with prism despite existing issues
  • Move to something like refractor (which is based on prism) or lowlight (which is based on highlight.js)
  • Add a configuration option ("slower but better" vs "faster but worse" highlighting)

I think any of these are reasonable and can be argued. Any specific opinions?

@lino-levan
Copy link
Contributor Author

I created this benchmark to test highlighting a large file.

import {
  all,
  createStarryNight,
} from "https://esm.sh/@wooorm/starry-night@2.0.0";
import { default as Prism } from "https://esm.sh/prismjs@1.29.0";
import "https://esm.sh/prismjs@1.29.0/components/prism-typescript";

Deno.bench("no-op", ()=>{})

const code = await (await fetch("https://esm.sh/v127/vscode-textmate@9.0.0/es2022/vscode-textmate.mjs")).text()
const starryNight = await createStarryNight(all);

Deno.bench("starry-night", () => {
  const scope = starryNight.flagToScope("ts")!;
  starryNight.highlight(code, scope);
})

Deno.bench("prism", () => {
  Prism.highlight(code, Prism.languages["ts"], "ts");
})

It is unacceptably slow. Closing this PR.
image

@lino-levan lino-levan closed this Jul 15, 2023
@hashrock
Copy link
Contributor

Thanks for measuring! this kind of information is very helpful

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.

yaml parsing does not work
3 participants