-
-
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
Cleanup unused JSX code #11741
Cleanup unused JSX code #11741
Conversation
🦋 Changeset detectedLatest commit: 84d91d7 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 |
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.
This PR is blocked because it contains a major
changeset. A reviewer will merge this at the next release if approved.
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.
This change must be reflected in the following places:
serverEntrypoint: 'astro/jsx/server.js', |
astro/packages/integrations/mdx/src/index.ts
Lines 37 to 42 in d12dcbf
export function getContainerRenderer(): ContainerRenderer { | |
return { | |
name: 'astro:jsx', | |
serverEntrypoint: 'astro/jsx/server.js', | |
}; | |
} |
The first one will be removed, and the second is already updated? |
@bluwy Is there a linear task in the 5.0 project associated with this breaking change? I think this should be added to the breaking changes section of the upgrade guide, and I've been tracking that we don't miss any of those by Linear issue number. |
Sorry this is related to PLT-1935 but I forgot to link it back. Yeah I'm planning to write this in the migration guide once the docs/changesets here are reviewed. |
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 @bluwy ! I don't think you need to go full "breaking change upgrade guide format" on the Astro major, but I still would like that in the 5.0 upgrade guide when it comes time! Some thoughts below for your consideration!
Co-authored-by: Sarah Rainsberger <[email protected]>
Changes
linear issue: PLT-1935
Removes unused code as previously I've refactored the logic into
@astrojs/mdx
already:astro/jsx/babel.js
astro/jsx/component.js
astro/jsx/index.js
astro/jsx/renderer.js
astro/jsx/server.js
(moved to@astrojs/mdx/server.js
)astro/jsx/transform-options.js
There's only
astro/jsx/rehype.js
left as it includes code tied to the astro metadata by Vite plugins. We could move it into@astrojs/mdx
too, but that may expose more internal stuff than wanted.Testing
Existing tests should pass. I also deleted some outdated jsx test for this.
Docs
I added a major changeset to
astro
, and only a minor for@astrojs/mdx
as the change there is technically not breaking. I expect that this change shouldn't affect many people as long as they have@astrojs/mdx
v3, which was released since 9th May 2024.