[react-scripts] Fix module resolution for yarn linked components

[yhnavein]

Hi,

Let me describe briefly our situation first.
We are using React applications generated by CRA and we have a separate repository with components. This component repository is a monorepo handled for us by lerna (with yarn workspaces). Typescript is used everywhere. All components are having only peerDependencies - no dependencies.

This set up works pretty decently, but we struggled with strange issues when at least one component from monorepo is yarn linked into the CRA app. Linking components is an essential feature for us.

The problem appeared firstly by broken styled-components in our app and message that multiple instances of styled-components package being in use at once and indeed there were multiple occurences of this package (and others too). Using a bundler in component's code and defining external packages does have no effect.

Following PR contains a fix for our problem. While doing an investigation we found that other approach of a fix was already proposed (#5654), but sadly forgotten. And those 2 ways are generally very similar.

The problem is that in case of linked components, symlinks are created and by default Webpack resolves them as real paths. It means that while analyzing our custom package from monorepo, webpack is looking for dependent modules by going up and up in the folder hierarchy, but because it's a symlinked location he won't hit node_modules of our application first, but node_modules of the root of the component's monorepo. Webpack will include this is the bundle and moves on. Later webpack will hit our application's use of the problematic 3rd party lib, will do the same but it will reach app's node_modules and tadam - we have multiple versions of basically the same lib bundled. I hope that I understood and explained it well :)

Turning off symlinks is one way. We changed slightly a module resolution by the webpack using a full path to application's node_module first. In our opinion it is less a breaking change. Optionally I can introduce an env variable that would prevent this from being a breaking change at all.

Topic is open for discussion.

Thanks

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[stristr]

Hey @yhnavein—this does indeed solve the problem I addressed in #5654 and I agree it should be less controversial.

I'll close that PR in favor of this one and upvote. Maintainers, please look at this issue!

[arutkowski00]

@yhnavein It looks like some tests may need adjustment.

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[stristr]

Keep alive!

[SRandazzo]

After struggling with this issue for quite some time, the fix in this Pull Request resolved my issue, thank you! How can we help move this along?

[jamime]

@SRandazzo there are failing tests Integration › Webpack plugins › linked modules this is because the test-integrity module which is linked in e2e-kitchensink.sh appears to be broken.

[eps1lon]

Did I understand it correctly that #6207 would solve the same issue? If so #6207 should be preferred since it would also fix false positives in ModuleScopePlugin. That plugin is currently running on every linked package.

[stristr]

@eps1lon, did you mean to post that elsewhere or reference a different PR? (This is #6207.)

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[fabienbranchel]

Don't!

[domtoupin]

We came across the very same issue, and we tried to figure out what was going on, i.e. why the tests were failing.

After an intense investigation that lead us to get a better understanding of webpack and npm modules resolution, the reason why the tests fails boils down to this:

• The npx create-react-app command (through yarn) installs all the direct dependencies and all other dependencies directly under node_modules; this includes test-integrity version 1.0.0 and version-string 1.0.0
• We change directory to a temporary folder, in which we yarn add the test-integrity module version 2.0.1 which depends on version-string version 2.0.0
• Then we npm link the test-integrity module we just installed in the temporary folder in our newly created react app
• At this point, we have two versions of version-string under node_modules: we have version-string 1.0.0 directly under node_modules and version-string version 2.0.0 under node_modules/test-integrity/node_modules
• With 'node_modules' alone, webpack resolve version-string to be the one located under node_modules/test-integrity/node_modules whilst with the fix provided by this PR, webpack resolve version-string to the one directly under node_modules which is "incorrect"

The reason why we end up in this situation is that npm link does not update the transitive dependencies the way "yarn add" would do. Therefore we end up in a situation where rather than having the same version of a transitive dependency directly node_modules and under the node_modulesof the dependency proving this dependency, we end up with two different version of the transitive dependency, an 'orphan' directly under node_modules and the expected one under the node_modules of the dependency providing the transtive dependency. One way to work around this would be to temporarily remove the test-integrity dependency before using npm link. If we did that, all the tests would work as expected, albeit without having the version-string dependency direclty undernode_modules. Would that work? It sort of breaks the purpose of having the test-integrity in the first place but would prove that create-react-app can work with a linked module. Does this make sense?

[Pajn]

Does this make sense?

This test error shows a real problem. All node modules with transitive dependences with a different version than the hoisted one will break.

[willisplummer]

I just ran into this issue. I wonder if at the least there's a way to make it more visible that it's a problem with the way things are being linked rather than with the packages themselves?

[sushruth]

Absolutely waiting for this to be merged into possibly 3.3.1 rather than 3.4

[eddiemonge]

@yhnavein can you confirm @domtoupin's issue?

[chapati23]

More than 2 years later, this is still a problem. It is currently impossible to use create-react-app with one of the most popular component libraries (Material UI) with yarn link.

Only option is to eject and tweak webpack config 😞

Here's a fix if you consider ejecting: mui/material-ui#14760 (comment)

[fabienbranchel]

@chapati23

You can still use a script editing webpack file : not ideal but far more better than ejecting.
I work on a project since almost three years with this, no issue found.

I use a specific commands for link like this :

"fix-react-scripts": "node custom/react-scripts/fix.js",
"link-package": "npm link ../my-project && npm run fix-react-scripts"

fix.js :

const fs = require('fs')

const path =
  __dirname + '/../../node_modules/react-scripts/config/webpack.config.js'

const file = fs.readFileSync(path).toString()

const newFile = file.replace(
  "['node_modules', paths.appNodeModules]",
  "[require('path').resolve('node_modules'), 'node_modules', paths.appNodeModules]"
)

fs.writeFileSync(path, newFile)