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 react-refresh transform #56

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

Jinjiang
Copy link
Contributor

@Jinjiang Jinjiang commented May 6, 2024

Change the transform call from with .js suffix into with .jsx in order to ensure the useReactRefresh flag in the React plugin
https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react/src/index.ts#L184

@silvenon
Copy link
Collaborator

silvenon commented May 6, 2024

Thanks for your contribution, but the plugin you linked to is not the one that this MDX plugin is using, it's referring to @vitejs/plugin-react-refresh, so I don't think that modifying the id extension would make a difference.

If you're using this plugin with the latest version of the React Vite plugin, it's possible that you're also using a modern Vite version that this plugin doesn't support. The maximum supported version of @vitejs/plugin-react is v1.3.2 because vite-plugin-mdx supports up to Vite v2.

The ecosystem has already progressed so far past this plugin, is there anything stopping you from using @mdx-js/rollup? @vitejs/plugin-react even has an example with MDX.

@silvenon
Copy link
Collaborator

silvenon commented May 6, 2024

the plugin you linked to is not the one that this MDX plugin is using, it's referring to @vitejs/plugin-react-refresh, so I don't think that modifying the id extension would make a difference.

My bad, it's also taking the React plugin into account as well, apparently, but if you're using the correct version of @vitejs/plugin-react then it's also using imports to React to detect JSX, it's not only using the extension.

The change in this PR looks safe enough, I just need to find a way to verify whether it fixes a problem, feel free to lend me a hand by providing some context about the problem you were facing, and how you landed on this fix.

But I'd still like to know if you need to use vite-plugin-mdx over @mdx-js/rollup.

@silvenon silvenon self-requested a review October 7, 2024 07:56
Copy link
Collaborator

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

I managed to verify that this fix indeed fixes React refresh, I'm sorry it took so long.

@silvenon silvenon merged commit 141e348 into brillout:master Oct 7, 2024
@silvenon
Copy link
Collaborator

silvenon commented Oct 7, 2024

Published in v3.6.1 🚀 Thank you!

@Jinjiang Jinjiang deleted the jinjiang/fix-react-refresh branch October 9, 2024 01:55
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