-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Move MDX plugin re-ordering hack to MDX integration #7872
Conversation
🦋 Changeset detectedLatest commit: ea61231 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 |
Maybe we should enforce this via |
Good call, looks like most other integrations has the peer dep, will add it for MDX too. |
// HACK: move ourselves before Astro's JSX plugin to transform things in the right order | ||
const jsxPluginIndex = resolved.plugins.findIndex((p) => p.name === 'astro:jsx'); | ||
if (jsxPluginIndex !== -1) { | ||
const myPluginIndex = resolved.plugins.findIndex( | ||
(p) => p.name === '@mdx-js/rollup' | ||
); | ||
if (myPluginIndex !== -1) { | ||
const myPlugin = resolved.plugins[myPluginIndex]; | ||
// @ts-ignore-error ignore readonly annotation | ||
resolved.plugins.splice(myPluginIndex, 1); | ||
// @ts-ignore-error ignore readonly annotation | ||
resolved.plugins.splice(jsxPluginIndex, 0, myPlugin); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This hack has been what I've been doing at https://github.com/bluwy/whyframe/blob/72e34498748f0b45d9272c886e123efd4dd24e17/packages/jsx/src/index.js#L26-L37 too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a question, not a blocker :)
@@ -53,6 +53,9 @@ | |||
"unist-util-visit": "^4.1.2", | |||
"vfile": "^5.3.7" | |||
}, | |||
"peerDependencies": { | |||
"astro": "workspace:^2.9.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this become 3.0.0
before the release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changesets or pnpm will bump that before the release. We should definitely check our existing integrations also have that bumped before the initial release too.
Changes
... instead of doing it in Astro core.
Testing
Existing tests should pass. However,
next
might need to merge upmain
changes for the recent fixes to the MDX fails.Docs
n/a. This change should not be visible as long as they upgrade Astro core and MDX integration to latest