Fix shared dependencies loaded multiple times

[wurstbonbon]

Description

There are many open issues regarding shared dependencies (often react) that are loaded multiple times:
#446
#478
#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 49b6cae 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 49b6cae. 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 #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?

• Bug fix
• New Feature
• Documentation update
• Other

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

• Read the Code of Conduct and follow the Commit Convention guidelines.
• 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.

[flyfishzy]

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

[wurstbonbon]

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]

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]

Hi @wurstbonbon I just tried this with the exact same scenario as this: #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]

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]

@wurstbonbon any update on the PR?

[Shelrothman]

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