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

Use shikiji #8502

Merged
merged 11 commits into from
Oct 11, 2023
Merged

Use shikiji #8502

merged 11 commits into from
Oct 11, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 11, 2023

Changes

Use shikiji instead of shiki. Also added compat code so it should work as before

Testing

Existing tests should pass

Docs

Most docs around shiki should also apply to shikiji, so I don't think we need to update the docs. The ideal case is to also have shikiji's improvements upstreamed to shiki.

@changeset-bot
Copy link

changeset-bot bot commented Sep 11, 2023

🦋 Changeset detected

Latest commit: b491e6b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added feat: markdown Related to Markdown (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Sep 11, 2023
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Oct 6, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@bluwy bluwy marked this pull request as ready for review October 6, 2023 14:00
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

It looks good to me. I wonder if we could move shared code inside the internal-helpers package.

.changeset/few-peas-hunt.md Outdated Show resolved Hide resolved
packages/astro/components/Code.astro Show resolved Hide resolved
packages/astro/components/shiki.ts Show resolved Hide resolved
@sarah11918
Copy link
Member

With respect to docs, I would only expect we need to add docs to the site if:

  • someone upgrades to 3.3 and suddenly their site looks different, without doing anything (this would be a breaking change, and should be in the changeset)
  • someone has additional config options, or more options to existing config values, that they can set
  • there is something new they can do now that they couldn't before (or the opposite, some behaviour has been removed)
  • there is a different way to do something that they could previously do

Outside of these conditions, there's probably not much we would document? We don't tend to explain a lot of inner workings, but rather how to do things. If the user has to act differently, or will notice different behaviour even as the result of changing nothing, these are the two big flags for docs.

@bluwy
Copy link
Member Author

bluwy commented Oct 10, 2023

@sarah11918

  1. The site should look the same. The themes are copied from shiki from shikiji.
  2. The existing options stay the same. shikiji does have extra APIs, but it's not exposed to the user yet.
  3. For now there's no new or removed features.
  4. Existing patterns stay the same.

Probably the only visible change is the markup where style="color: #ffffff" is now style="color:#ffffff".

And <span class="line" style="color: #ffffff" is now <span class="line" + <code class="astro-code" style="color:#ffffff". It's a new strategy where span.line inherits the color from code instead. Maybe I can document this in the changeset, but I'm not really sure if it'll affect people a lot.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great, @bluwy ! Just some very minor changes to the opening line, but I think this is more than helpful for describing the change!

.changeset/few-peas-hunt.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <[email protected]>
@bluwy bluwy merged commit c4270e4 into main Oct 11, 2023
13 checks passed
@bluwy bluwy deleted the shikiji branch October 11, 2023 16:07
@astrobot-houston astrobot-houston mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants