Fix shared dependencies loaded multiple times

wurstbonbon commented 8 months ago

Description

There are many open issues regarding shared dependencies (often react) that are loaded multiple times: https://github.com/originjs/vite-plugin-federation/issues/446 https://github.com/originjs/vite-plugin-federation/issues/478 https://github.com/originjs/vite-plugin-federation/issues/429 ...

Here is my understanding what is going on and were the problems lies. Correct me if I’m wrong, I’m just stumbling through the code trying to make sense of it. 

The last version that work well with React and libraries that depend on React is 1.1.14. With 1.2.0 came this commit https://github.com/originjs/vite-plugin-federation/commit/49b6caebbef899abf18cd8d62f64261aac6a18cd It moved the transformation of the shared module imports from generateBundle to transform.

This is not working well together with the CommonJS plugin. The transform sees commonjsHelpers.js imports and imports like node_modules/react/index.js?commonjs-proxy. It does not transform those imports and they end up as direct imports instead of importShared() calls. This will then cause two instances of e.g. react to be imported one from the host and one from the remote.   

I made a ham-fisted attempt to fix this by adding a second import conversion round in generateBundle. This sort of reverts the changes done in https://github.com/originjs/vite-plugin-federation/commit/49b6caebbef899abf18cd8d62f64261aac6a18cd. Honestly I don’t think this makes a lot of sense, but it could be a starting point for a discussion. Note: The fix does not work for SystemJS. Because by the time generateBundle is called for SystemJS the imports from the CommonJS plugin are no longer recognisable as shared imports. 

The commit mentioned above was introduced to fix this issue https://github.com/originjs/vite-plugin-federation/issues/334 I’ve tried to reproduce the issue on the given example repo. Using 1.1.14 and even 1.1.12, but was unable to do so. So I don’t understand what the actually initial problem was. Could be that I'm doing something or that changes in vue cause this to no longer happen.

Maybe a way forward would be to more thoroughly revert back to the initial approach of doing the conversion in generateBundle.   

Additional context

What is the purpose of this pull request?

[x] Bug fix
[ ] New Feature
[ ] Documentation update
[ ] Other

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

[x] Read the Code of Conduct and follow the Commit Convention guidelines.
[x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
[x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
[x] Ideally, include relevant tests that fail without this PR but pass with it.

flyfishzy commented 8 months ago

Hi @wurstbonbon, it seems that CI is failed in serve mode. @ruleeeer , Can you take the time to review the code?

wurstbonbon commented 7 months ago

Hi @flyfishzy, I figured out why the CI was failing. Before my changes the webpack-host example was loading react twice. After my changes the remote tries to use react from the host. This was failing because the react module from webpack is structured differently. Again a common.js issue I'm afraid. I added a workaround by setting commonjsOptions: {defaultIsModuleExports: true} on the remote. I also had to make sure that every shared module exports default so that it can be dynamically imported.

I created a new example / test using react with zustand to demonstrate what has been fixed.

wurstbonbon commented 7 months ago

Hi @flyfishzy, thanks for retriggering the tests. Any chance that failing tests are just a fluke? I've run them locally on macOS with node 16 a couple of times and I can't reproduce the failure. Also strange that it would only fail on macOS with node 16.

acollazomayer commented 6 months ago

Hi @wurstbonbon I just tried this with the exact same scenario as this: https://github.com/originjs/vite-plugin-federation/issues/446. And the fix is not working maybe I am missing somethig. I need to add commonjsOptions: {defaultIsModuleExports: true}

Happy to help.

I still have the same error Uncaught Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:

wurstbonbon commented 6 months ago

Hi @acollazomayer, are both host and remote using vite? I had to add commonjsOptions: {defaultIsModuleExports: true} when using a webpack host.

How is what you are doing different from the example that I added?

I tried sharing react-router-dom works fine.

satyam-veris commented 6 months ago

@wurstbonbon any update on the PR?

Shelrothman commented 6 months ago

@wurstbonbon any updates on this PR? It seems to only be failing on one test now; CI / Build&Test: node-16, macos-latest

wurstbonbon commented 5 months ago

Great that there is finally something happing here 👍

@Shelrothman
As I mentioned before I'm not sure if the failing node-16 macOS tests aren't a fluke. I even tried it with node-16 on a Mac and the tests were successful. Can we re-run the tests? If there is actually a node-16 + macOS specific issue then I'm afraid I need help.

ReCREWt-Fabi commented 5 months ago

@wurstbonbon Any updates on this PR? We'd urgently need this fix to be released to get reactflow to work in a remote component

Shelrothman commented 5 months ago

@wurstbonbon Any updates on this PR? We'd urgently need this fix to be released to get reactflow to work in a remote component

unfortunately, there are no updates as far as I know. My team has had to shift to a new build tool. Now we are trying to use rsBuild and @module-federation/enhanced/rspack to do the "micro-frontending". Although it has its own issues too..

jean-moreira-ihm commented 4 months ago

Do you have any news about this fix? Will it be applied, or are there alternatives that can be done until this solution is implemented?

pganster commented 2 months ago

@Shelrothman can you highlight problems you had with rsbuild? We are now considering to switch too, as this repository seems to have become stale, as the last change was 5 months ago. I also don't believe that this PR will ever be merged considering the activity.

Shelrothman commented 2 months ago

@Shelrothman can you highlight problems you had with rsbuild? We are now considering to switch too, as this repository seems to have become stale, as the last change was 5 months ago. I also don't believe that this PR will ever be merged considering the activity.

Hey @pganster unfortunately, I don't think I can be of much help. rsBuild is fine if all the remote apps use the same build-tool, but ours do not, so it didn't work out. My team decided to shift away from trying to micro-frontend for the time being. Instead we are focused on building out a shared component library (via npm package) and have all our apps consume the library. To give a "look" of microfrontending without actually doing it. 🙈

We may revisit later on, but way too much developer time has already been spent on it so it wont be for a while most likely.

pganster commented 1 month ago

Just a heads up: We've now successfully switched all our services from vite + this module federation plugin to rsbuild. RsBuild builds upon the module-federation/core and is for now better maintained. For now we didn't encounter any major issue that prevented us from switching.

originjs / vite-plugin-federation