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: migrate to Shikiji #109

Merged
merged 21 commits into from
Dec 3, 2023
Merged

feat: migrate to Shikiji #109

merged 21 commits into from
Dec 3, 2023

Conversation

atomiks
Copy link
Collaborator

@atomiks atomiks commented Sep 24, 2023

Closes #107

@netlify
Copy link

netlify bot commented Sep 24, 2023

Deploy Preview for rehype-pretty-code ready!

Name Link
🔨 Latest commit e4a4f73
🔍 Latest deploy log https://app.netlify.com/sites/rehype-pretty-code/deploys/656c2ab6582ca2000824f45d
😎 Deploy Preview https://deploy-preview-109--rehype-pretty-code.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dimaMachina
Copy link
Contributor

I’ll try to take a look this week if I can help

@ovflowd
Copy link

ovflowd commented Oct 26, 2023

Would love if this gets merged. Right now Shiki is not compatible with Next.js 14 or Turbopack or SWC... And using Shikiji would def solve many problems 🙇

@atomiks atomiks force-pushed the feat/shikiji branch 2 times, most recently from 493e255 to 4f3a918 Compare December 1, 2023 01:52
@atomiks
Copy link
Collaborator Author

atomiks commented Dec 1, 2023

Think I managed to get this working. There are some API changes so it's partially breaking but most things are the same still

@atomiks
Copy link
Collaborator Author

atomiks commented Dec 2, 2023

I found a really weird bug with the md markdown language testing the beta on the website. It seems to be a Shikiji issue.

If the md language is registered before others, it ends up glitching them out:

```md
This is markdown block
```

```js
console.log('will not be highlighted');
```

`console.log('this even throws an error'){:js}`

Ensuring md is always registered last seems to fix the issues.

if (langsToLoad.has('md')) {
  langsToLoad.delete('md');
  langsToLoad.add('md');
}

I need to see how this also works on the client.

@atomiks
Copy link
Collaborator Author

atomiks commented Dec 2, 2023

  • shikiji@0.7.6 fixes that issue above
  • I added a transformers option (top-level), which works with the shikiji-transformers package; I've added test for it. So I think this PR: Adds Diff Highlighting #124 is no longer necessary @IanMitchell

@IanMitchell
Copy link

Perfect! Closed my PR, thank you!

@atomiks
Copy link
Collaborator Author

atomiks commented Dec 2, 2023

Thanks!

I'm going to keep the onVisit* hooks since transformers don't deal with some custom nodes this package adds.

I think this is mostly ready, but I've published 0.12.0-beta.1 to test out

src/index.ts Outdated Show resolved Hide resolved
@@ -1,4 +1,10 @@
import type { Highlighter, IShikiTheme } from 'shiki';
import type {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why do you manually declare types? And don’t generate them based on src/index.ts file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be generated yeah :) Originally, the source was in plain JavaScript but I didn't update the way types were generated when I converted the codebase to TypeScript.

src/index.ts Outdated Show resolved Hide resolved
lang as Parameters<typeof highlighter.loadLanguage>[0],
);
} catch (e) {
return Promise.reject(e);
Copy link
Contributor

@dimaMachina dimaMachina Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try catch block can be completely removed (with keeping code from try block) since you just re throw error in catch block?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed because if one of the languages throws then it should still continue with the rest

"rome": "12.1.3",
"shiki": "^0.14.0",
"rollup": "^4.6.1",
"shikiji-transformers": "^0.7.6",
"ts-jest": "^29.1.0",
"typescript": "^4.9.5",
"vite": "^4.3.9",
"vitest": "^0.32.2"
},
"dependencies": {
"@types/hast": "^3.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in dev dependencies?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure if Shikiji itself should be a peer (locked to minor version though) - but this was just for testing purposes. The user would need to install it themselves (but again, might be a problem with versions)

package.json Outdated Show resolved Hide resolved
@atomiks atomiks merged commit b682355 into master Dec 3, 2023
5 checks passed
@atomiks atomiks deleted the feat/shikiji branch December 3, 2023 07:19
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.

migrate/support shikiji
4 participants