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(bridge-react): prevent react-dom/client being bundled #3476

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

francisduvivier
Copy link

@francisduvivier francisduvivier commented Feb 3, 2025

This otherwise causes the package to be incompatible with a react 19

BREAKING CHANGE: react-dom/client code will not be bundled into the bridge-react module anymore

Description

This PR changes the vite rollup config for the bridge-react package in order not to bundle react-dom/client.
I assume that this inclusion into the bundle was accidental, since react-dom/client is part of react-dom which is a peer dependency, and peer dependencies are marked as external already in the rollup config.

The inclusion of this v18 react-dom/client module code also hinders compatibility with remote react v19 modules that want to use react bridge. So this PR fixes that part of imcompatibility with react v19, I don't know if there is other issues with react v19 though.

To see what I mean, you can look at the bridge-react/dist/index.es.js

Technically this fix can be breaking if someone somehow depends on the react-dom/client code being bundled into the @module-federation/bridge-react package.

Related Issue

#3477

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

Copy link

changeset-bot bot commented Feb 3, 2025

🦋 Changeset detected

Latest commit: f29b319

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 29 packages
Name Type
@module-federation/bridge-react Patch
@module-federation/runtime Patch
@module-federation/enhanced Patch
@module-federation/rspack Patch
@module-federation/webpack-bundler-runtime Patch
@module-federation/sdk Patch
@module-federation/runtime-tools Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/dts-plugin Patch
@module-federation/third-party-dts-extractor Patch
@module-federation/devtools Patch
@module-federation/bridge-vue3 Patch
@module-federation/bridge-shared Patch
@module-federation/bridge-react-webpack-plugin Patch
@module-federation/modern-js Patch
@module-federation/retry-plugin Patch
@module-federation/data-prefetch Patch
@module-federation/rsbuild-plugin Patch
@module-federation/error-codes Patch
@module-federation/inject-external-runtime-core-plugin Patch
@module-federation/nextjs-mf Patch
@module-federation/node Patch
@module-federation/storybook-addon Patch
@module-federation/modernjsapp Patch
@module-federation/esbuild Patch
@module-federation/runtime-core Patch
@module-federation/utilities Patch
website-new Patch

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

Copy link

netlify bot commented Feb 3, 2025

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit f29b319
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/67a1becec8e16800084d749d
😎 Deploy Preview https://deploy-preview-3476--module-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…ndex.*.js

this otherwise causes the package to be incompatible with a react 19

BREAKING CHANGE: react-dom/client code will not be bundled into the bridge-react module anymore
@francisduvivier francisduvivier force-pushed the fix/bridge-prevent-react-dom-client-bundling branch from 313644e to f29b319 Compare February 4, 2025 07:16
@francisduvivier
Copy link
Author

I skipped some of the steps like adding tests since that seems a bit much for this, but let me know if some extra work is needed from my side for this to be merged.

@danpeen
Copy link
Contributor

danpeen commented Feb 5, 2025

@francisduvivier hello, thanks for your pr. And I have checked @module-federation/bridge-react packages bundle config, i think in the rollupOptions.external configs we have already set the peerDependencies as external packages, which of course include react-dom and should not be bundled into the final assets. So i think react-dom/client will not be bundled either. So i was wondering how you find that react-dom/client packages being bundled and if it is i think that is not expectable, please give me an example so i can offer some help.

@francisduvivier
Copy link
Author

francisduvivier commented Feb 5, 2025

Hi @danpeen, that is a good question, I have tried my best to specify it in the reproduction part in the related issue:
#3477

Reproduction

  • Check out this repository

  • build the bridge-react package

  • check the imports in bridge-react/dist/index.es.js

    • You will see that there is no import for react-dom/client, instead the code is bundled below containing __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED on line 333
  • Alternatively to the steps above you can also check the bundle at https://www.npmjs.com/package/@module-federation/bridge-react?activeTab=code and then @module-federation/bridge-react/dist/index.es.js line 333

@danpeen
Copy link
Contributor

danpeen commented Feb 6, 2025

Hi @francisduvivier, i got what you mean. Here is the thing:

image

  • this piece of code wraps the entry point of react-dom/client, mainly wrapping two methods: createRoot and hydrateRoot.
  • it is a wrapper layer, rather than directly packaging react-dom/client.
  • the key point is the m variable, which will be imported from the outside as a react-dom, not packaged in.

and you can try even if we set the react-dom/* as the external packages as the pr changes, you will see it will act the same as the old config.

and if we truly bundled the react or react-dom into the final assets, i think you can find code like this
function createRoot(container, options) { ... } which contains a large number of specific API's implementation

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