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

fix: compatibility with react-refresh runtime in @vitejs/plugin-react #20

Merged
merged 4 commits into from
Dec 13, 2022

Conversation

ElMassimo
Copy link
Contributor

@ElMassimo ElMassimo commented Dec 12, 2022

Description 📖

This pull request fixes compatibility with the runtime served by @vitejs/plugin-react.

Background 📜

The preamble used in the @vitejs/plugin-react plugin imports the default export from the runtime, calling injectIntoGlobalHook from it.

In contrast, the runtime served by this plugin does not provide a default export.

This affects compatibility with external integrations, such as Vite Ruby and the Laravel Vite integration, which were implemented to use the same preamble as the one used by @vitejs/plugin-react.

The Fix 🔨

Adding a default export to the runtime served by this plugin.

It doesn't affect backwards compatibility, so it can be shipped in a patch release.

Notes ✏️

Although we could potentially modify the @vitejs/plugin-react plugin instead to use named exports, it would remain incompatible with previous versions of the backend integrations and other consumers of the runtime.

This change has significantly less friction, and would allow any users of @vitejs/plugin-react in backend integrations to seamlessly switch to this plugin.

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Dec 12, 2022

Thanks for the PR and the background explanation. I don't particularly like default export but in the case of pure ESM for client that's fine for me.

Can you add a comment before the default export like // For compatibility with Babel plugin (#20) and a line in the changelog?

@ElMassimo
Copy link
Contributor Author

ElMassimo commented Dec 12, 2022

Will do, although it would be nice to configure automatic changelog generation as in the main Vite repo, it will automatically link related commits and PRs 😃

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Dec 12, 2022

We're discussing that with the team and we will try to move in the other direction. If you don't read esbuild releases notes, you should subscribe, there are really well crafted and after that you really dislike generated changelog!

Edit: For example in the changelog you can says more than in a commit title and tell that this fixes integration for Ruby/Laravel, which you will more clear for users that could have encounter the problem and just discarded the migration after an error that they didn't understood.

@ArnaudBarre ArnaudBarre merged commit 6b4759f into vitejs:main Dec 13, 2022
@ElMassimo ElMassimo deleted the fix/react-refresh-compat branch February 16, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants