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

Support Bun by adjusting how @babel/plugin-transform-react-jsx is imported. #8007

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

paperdave
Copy link
Contributor

Changes

This brings Astro a step closer to running on Bun by working around an existing workaround done when loading @babel/plugin-transform-react-jsx.

In Bun and some other tooling, importing a CommonJS module that has module.exports.__esModule will tell the module loader/bundler to set module.exports.default as the ESM default export, where without this annotation the default export will be set to module.exports. Node does not do this transformation, which turns transpiled-to-cjs modules like @babel/plugin-transform-react-jsx into { default: { default: Function } }, where in bun this loads as originally intended like { default: Function }

This PR fixes the three places astro uses .default.default and ensures it handles the case where it may be just .default.

Our tracking issue for Astro as a whole is oven-sh/bun#3746. This PR doesn't fully close that yet, as there are some remaining stability issues in Bun (random segfaults) when using larger projects.

Testing

I have been manually testing this on the astro.build docs locally on macos with node.js v20.5.0 and a development build of bun.

Docs

This is an internal change to the codebase and has no user-facing side effects. Maybe I should've added a comment for future code readers to understand why checking both .default.default and .default is necessary.

@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2023

🦋 Changeset detected

Latest commit: 6b48747

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 pkg: preact Related to Preact (scope) pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Aug 9, 2023
@matthewp
Copy link
Contributor

matthewp commented Aug 9, 2023

I'm not sure if we should add special-casing code for Bun. I could see someone looking at this code and thinking it's unnecessary and removing it.

@natemoo-re
Copy link
Member

I'm not sure if we should add special-casing code for Bun. I could see someone looking at this code and thinking it's unnecessary and removing it.

I don't mind this change, personally! It seems trivial to support Bun's divergent behavior here. Since the ultimate goal for Bun is full Node compat, the rest of compatability issues will be their responsibility.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

LGTM. Worth noting that this code is changing in #7924 for 3.0, but this is worth merging regardless IMO!

@natemoo-re natemoo-re merged commit 58b121d into withastro:main Aug 9, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pkg: preact Related to Preact (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants