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

Bump shikiji, use transformers API, expose transformers API #9643

Merged
merged 14 commits into from
Jan 17, 2024

Conversation

blackmann
Copy link
Contributor

Changes

  • Bump shikiji version
  • Use transformers API. tranforms is deprecated.
  • Expose the transformers API so that some tranformer presets or community transformers can be used.

Testing

There's are existing tests for this and they pass.

Docs

We'll need to update the docs to indicate that transformers can be supplied to astro.config.js > markdown.shikiConfig.transformers

/cc @withastro/maintainers-docs for feedback!

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: 7d24b32

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 the feat: markdown Related to Markdown (scope) label Jan 8, 2024
@ematipico
Copy link
Member

@blackmann please a have at the CI, it seems TypeScript can't build the project

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Jan 8, 2024
@blackmann
Copy link
Contributor Author

blackmann commented Jan 8, 2024

I'm getting this error when trying to build astro

astro:build: src/core/config/schema.ts(67,14): error TS2742: The inferred type of 'AstroConfigSchema' cannot be named without a reference to '.pnpm/[email protected]/node_modules/shikiji-core/dist/chunk-types.mjs'. This is likely not portable. A type annotation is necessary.

Any idea what it means. It's hard trying to understand what it means.

I don't know much about how pnpm works; I'm unable to find the shikiji-core package in any of the node_modules though it's stated as a dependency for shikiji. I dunno if that's the cause.

Ideas? @ematipico


Edit:

I imported the types into schema.ts and seems to work. ❇️

@blackmann
Copy link
Contributor Author

 Tasks:    16 successful, 16 total
Cached:    9 cached, 16 total
  Time:    4m22.645s

All tests passing locally for me. I can't tell what the failure is with the CIs.

@ematipico please have a look¿

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I've pushed some commits to fix the test and some clean-ups. And it looks good to me now. Marking this as minor to be merged on our next minor release push.

For the documentation, we can update https://docs.astro.build/en/guides/markdown-content/#shiki-configuration to add the new option.

@bluwy bluwy added the semver: minor Change triggers a `minor` release label Jan 10, 2024
@ematipico ematipico added this to the 4.2.0 milestone Jan 10, 2024
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.

Blocking, as it will be part of our next minor release. We will merge it in the next merge day :)

packages/astro/package.json Outdated Show resolved Hide resolved
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.

If this adds a new user-configurable option, there needs to be documentation and an example showing this. Where is the config set (code snippet)? What are the values you can set, and what do they do?

I'm going from the description provided in the changeset. If this is not user-configurable, then "exposes a new config option" is not the right way to describe this change, because it implies that there is something you can do here!

.changeset/six-scissors-worry.md Outdated Show resolved Hide resolved
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.

Blocking this, because we have no documentation.

We will need:
- JSDoc content to update the configuration reference page automatically <- looks like we don't go into that level of Shiki config here, so only the docs PR is required!

For a minor release, not opposed to showing a code snippet of the config in the changeset if you so decide, either!

@ematipico
Copy link
Member

ematipico commented Jan 15, 2024

@blackmann could you please help us with documentation? If not provided, we won't be able to release this PR on Thursday

@blackmann
Copy link
Contributor Author

Yes. On it!

@blackmann
Copy link
Contributor Author

Updated docs: withastro/docs#6402

@ematipico @sarah11918

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.

Just some small light edits to the changeset, for your consideration, but otherwise docs is happy!

.changeset/six-scissors-worry.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 dismissed their stale review January 15, 2024 19:04

docs received!

Co-authored-by: Sarah Rainsberger <[email protected]>
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.

Final approval for docs!

@ematipico ematipico merged commit e9a72d9 into withastro:main Jan 17, 2024
12 of 13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 17, 2024
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.

5 participants