Skip to content

feat: add reactRefreshHost option to support module federation HMR #420

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

Merged
merged 7 commits into from
Apr 11, 2025

Conversation

protoEvangelion
Copy link
Contributor

@protoEvangelion protoEvangelion commented Mar 26, 2025

Description

This resolves HMR not working in a module federation context when using the @module-federation/vite plugin.

This PR simply adds a new option reactRefreshPrefix in a backwards compat way to allow prefixing the react-refresh url which by default is: /@react-refresh

The reason this works is the following example:

  1. host vite server on port 3000
  2. remote vite server on port 4000
  3. save file in remote server
  4. react-refresh triggers on remote server port 4000 but does not propagate to the host server

Allowing dev to set prefix in the plugin will force the react-refresh plugin from remote files to point at the host react-refresh module:

react({ reactRefreshPrefix: 'http://localhost:3000' })

The react refresh url becomes http://localhost:3000/@react-refresh.

Screen.Recording.2025-03-25.at.8.23.09.PM.mov

Additional context

module-federation/vite#183 (comment)


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@protoEvangelion
Copy link
Contributor Author

@ArnaudBarre this is my first PR here, let me know if everything looks good 😅

Copy link
Member

@ArnaudBarre ArnaudBarre left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of the complexity added by module federation, but this small feature doesn't cost much to add so why not.
@sapphi-red @patak-dev @bluwy @hi-ogawa opinions?

@sapphi-red
Copy link
Member

Adding this looks fine to me for the same reason with you. I'd like to hear from @underfin what they think about it as they have been working on MF related stuff recently.

@bluwy
Copy link
Member

bluwy commented Mar 27, 2025

Just wondering as I'm not clear of the relationship of the host & remote server, does relying on Vite's server.origin option help here? Maybe plugin-react could generate the import that takes in account of that.

@ArnaudBarre
Copy link
Member

@bluwy Interesting, I'll try to add a test setup in the repo for this usecase so that I can try out

@protoEvangelion
Copy link
Contributor Author

Thanks for the quick feedback & good suggestions @ArnaudBarre! I just committed all your requested changes, tested, & works as expected.

@sapphi-red I'm super curious about that as well. However for those of us using module federation in a multi build system, this is still necessary. I would love to vite all the things but I'm living in a world that still webpacks all the things 😅

@bluwy Thanks for the suggestion. I don't think that will work for assets on the remote server because if I change the origin, it will look for assets on the host server which won't be there.

@protoEvangelion protoEvangelion changed the title feat: add backwards compatible option reactRefreshPrefix to support module federation HMR feat: add reactRefreshHost option to support module federation HMR Mar 28, 2025
@protoEvangelion
Copy link
Contributor Author

@ArnaudBarre I think we are all clear to merge this. Anything else you need me to address?

@ArnaudBarre
Copy link
Member

@sapphi-red I let you decide when you prefer to merge given your other open PRs. I think we could also add this to other plugins

@protoEvangelion
Copy link
Contributor Author

@sapphi-red @ArnaudBarre I rebased/updated according to your last merged pr moving code to common. Tested & works as expected. Side question, are you planning to do a release anytime soon? Because I haven't seen one since November 2024.

@ArnaudBarre
Copy link
Member

Thanks for updating your PR! We didn't release anything in the past months because no new bug fix or feature was merged, but we will do a release shortly after merging this.
If you want you can also add this option the SWC plugin, but this is not required to merge at all

@protoEvangelion
Copy link
Contributor Author

Makes sense 👍 I fixed conflicts again and added changes to plugin-react-swc as well.

@protoEvangelion
Copy link
Contributor Author

Fixed conflicts & edited readme + changelog per review comments.

@underfin
Copy link
Member

Adding this looks fine to me for the same reason with you. I'd like to hear from @underfin what they think about it as they have been working on MF related stuff recently.

I think it is a good idea to support mf react hmr. The host and remote could be use host react-refresh runtime. The previous bug caused by the remote react-refresh runtime is not called RefreshRuntime.injectIntoGlobalHook(window). After the pr the issue is gone. Thank you @protoEvangelion nice work.

@sapphi-red sapphi-red merged commit 87f7fdd into vitejs:main Apr 11, 2025
7 checks passed
@protoEvangelion protoEvangelion deleted the react-refresh-prefix branch April 11, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants