verifyPackageTree() errors when wrong version dependency is installed in any parent directory

[newoga]

Is this a bug report?

Yes

Did you try recovering your dependencies?

No. This issue presents an argument that there may be a bug in the verifyPackageTree.js logic added in the next branch.

Which terms did you search for in User Guide?

version, dependenc. This does not appear to be documented (and since it is internal, maybe it shouldn't be). There is discussion regarding documenting some of the packages related to this in issue in issue #4137 (suggested by @Timer here).

Environment

$ node -v
v8.9.4

$ npm -v
5.7.1

$ yarn --version
1.5.1

Running on macOS 10.13.2.

Steps to Reproduce

I created a reproduction project here: https://github.com/newoga/create-react-app-issue-4167

The project is a simple node package that depends on a version of jest that is incompatible with [email protected]. The project also contains a directory (cra-app) that was generated by create-react-app. The react-scripts dependency in that sub-project has been updated to 2.0.0-next.47d2d941.

1. git clone https://github.com/newoga/create-react-app-issue-4167
2. cd create-react-app-issue-4167
3. yarn
4. yarn run

Expected Behavior

From a user perspective, the yarn run command should not error and the application should start. The user did not manually install an incompatible version of jest in the create-react-app generated project. The version of jest in the generated project's node_modules is the correct version and parent directories should not have an impact.

From a technical perspective, the verifyPackageTree.js logic should see that the cra-app project contains the correctly installed version of jest and stop checking parent directories. Parent directories should only be traversed if jest is not installed.

Actual Behavior

An error occurs because parent directory depends on a version of jest that is incompatible with the version of jest that create-react-app generated project depends on.

Reproducible Demo

https://github.com/newoga/create-react-app-issue-4167

Edit: Updated the issue number on the links.

[newoga]

Just some additional context, I ran into this issue while testing and experimenting with the monorepo/yarn workspaces support in the next version. The side effect of having a create-react-app based project in a monorepo in this way is that all packages must share the same version of the handful of dependencies that create-react-app enforces.

[Timer]

/cc @gaearon was this intentional? I can't recall.

[brunolemos]

I just faced this when trying out react-script@2 for the first time.
It definitely seems like a bug.

My project structure:

• project
  • node_modules (jest 23, for rn 0.56 and typescript)
  • android
  • ios
  • web
    • node_modules (jest 22)

Created an .env file with SKIP_PREFLIGHT_CHECK=true for now to ignore this.

All tests pass just fine when running yarn test.

[transitive-bullshit]

I am also running into this in a lerna monorepo with npm (not yarn). ("react-scripts": "^2.0.0-next.3e165448")

We have one version of eslint installed for running lint-staged at the root of our monorepo and then within our CRA app's package, it installs the react-scripts eslint.

This definitely seems like CRA trying to be too strict in its enforcement. Instead of checking every folder in the node module resolution path, wouldn't it be enough to check require.resolve(<moduleName>) which would find the closest node_modules version?

[newoga]

/cc @gaearon was this intentional? I can't recall.

@Timer No pressure, but do you know if we confirmed if this behavior is intentional or not? I don't mind taking a stab at fixing this if it's agreed that this needs to change. Thanks!

[Timer]

This is the intentional behavior. We're going to make an E2E test case to showcase why.

[newoga]

@Timer Okay, thanks for the quick response. Interested to see why but I will wait for the E2E test case. Feel free to close this issue or use is as a "reminder" for the E2E test case.

[Experiment8]

Hello,

I think I'm encountering the same issue on a CRA based app, but is slightly different from what described above. Here are some details about it.

Error message:

The react-scripts package provided by Create React App requires a dependency:

"babel-eslint": "9.0.0"

Don't try to install it manually: your package manager does it automatically.
However, a different version of babel-eslint was detected higher up in the tree:

app/node_modules/babel-eslint (version: 8.2.6) 

This is the yarn list situation:

├─ [email protected]
├─ [email protected]
│  └─ [email protected]
└─ [email protected]
   └─ [email protected]

Apparently is checking for the highest up in the tree, which would make sense, but the 8.2.6 is there because of CRA dependencies, Here's what I gathered from the .lock file.

|- [email protected]
    |- [email protected]
        |- cssnano@^4.1.0
            |- cssnano-preset-default@^4.0.2
                |- postcss-merge-longhand@^4.0.6
                    |- stylehacks@^4.0.0
                        |- postcss-selector-parser@^5.0.0-rc.3
                            |- babel-eslint@^8.2.3
                                version "8.2.6"

[w-biggs]

I have this issue as well. If it's intended behavior, is there a workaround other than putting the parent project in a sibling directory instead?

[ghost]

@Experiment8 I am facing the same error when I am building the react apps. I tested that with the basic template. But, still getting this same error as you. Did you happen to rectify this error in your application?

[nicojs]

Yes same error here. Quick steps to reproduce:

mkdir ~/tmp
cd ~/tmp
npm i -D [email protected]
npx create-react-app foo
cd foo
npm t

[Timer]

@nicojs as mentioned above, this is the desired behavior -- not a bug.

[negebauer]

@Timer Any news on the E2E test case to showcase why you mentioned?

[nicojs]

My tiny brain has problems to understand why it would be a problem, but if it is desired behavior an e2e case would help to explain why. The 4 steps to resolve the issue don't help for this issue as well. A mention of this issue with the reason why would be greatly appreciated.

[Timer]

The problem would generally showcase in a monorepo when combined with hoisting:

Say you have a package abc that relies on foo and webpack@4, but a package def and ghi that relies on webpack@3.

The package manager hoists webpack@3 to the root because it's used 2x, where webpack@4 is only used once.
foo is also hoisted because there's only one instance of it across your project.

Now, when foo requires webpack, it receives webpack@3 instead of webpack@4. This check is designed to prevent this behavior, and needs to be overly eager because we don't know the list of packages which may require webpack.

/node_modules/webpack@3
/node_modules/foo
/packages/abc/node_modules/webpack@4
/packages/def/node_modules/<empty>
/packages/ghi/node_modules/<empty>

This same problem exists when a user self-installs webpack within a single project, causing webpack nesting in the tree.

[nicojs]

So it isn't a problem when the parent directory is not part of a monorepo or when you decide to not hoist the dependencies? If so, we could maybe build in this check? Right now, it's pretty much all or nothing.

Side note: this hoisting business seems like a bad idea to me if it can break the dependency system.

[Experiment8]

@Timer thanks for the example, but I still believe something isn't quite right with how the hoisting is working. E.g. in my case, the only dependency requiring babel is actually CRA (tried to remove the eslint package to check) but still the build check fails for what I shown before, preventing the production build.

Disabling the check and let the build go produces a perfectly working build tho, so no problem with deps resolving.

[negebauer]

Maybe instead of just failling it could check which packages may have problems with hoisting. In your example, foo

So it would output a message saying something like “you need to add foo to your no hoist configuration” to avoid that dependency problem

That’s the case for a project I was working. We had to disable hoisting for two packages to avoid this problem, but still need to skip preflight in order to run the app

[Timer]

I agree we can make this smarter -- maybe we can try to detect when in a workspace/monorepo and bail out once we hit the root of the workspace.

[andyfleming]

I've got a similar use case to @brunolemos. It seems like a common project structure that we intend on supporting. (The proxy feature in CRA, for example, supports this project structure).

I'm similarly using SKIP_PREFLIGHT_CHECK=true to work around the issue.

[newoga]

@Timer I don't fully understand the concern with hoisting based on your example here, assuming I am not misunderstanding how yarn's hoisting works.

I recreated the example in a new branch. You were right that since webpack@3 is required more than webpack@4, webpack@3 is hoisted to the root directory. However, webpack@4 is still installed "locally" in the node_modules directory for the one package/workspace that depends on it.

Based on this hoisting behavior, create-react-app should not need to traverse and validate versions of the dependencies in all parent directories. It should only need to validate versions for the first occurrence of each dependency when traversing up the directory hierarchy (in order to mimic node's package resolution behavior).

Let me know if I'm misunderstanding the problem we are trying to avoid/protect end users from.

Edit: tl;dr; Are we sure we cannot (1) Trust yarn to hoist dependencies properly in a way that does not impact create-react-app, and (2) Change verifyPackageTree() validation logic to only validate the dependencies that would actually be resolved by node's package resolution logic?

[simonbuchan]

Regarding @Timer's example, which package would foo be in this case - if it's react-scripts wouldn't it always resolve webpack to it's direct dependency? If it's trying to specifically enforce that sub-packages with peer dependencies get exact versions, shouldn't they be bundledDependencies?

[pietmichal]

@newoga, your example misses one crucial detail - the foo package mentioned by @Timer.

What will happen if you install a foo package that requires webpack@4 as a dependency?

According to this blog post - https://yarnpkg.com/blog/2018/04/18/dependencies-done-right/ - webpack@4 is installed in node_modules of foo package making your point valid.

But what will happen if you install a package that requires webpack@4 as a peerDependency?

According to the blog post above, it will target webpack@3 leading to a potential failure.

This is what @Timer tried to explain.

In my opinion, react-scripts shouldn't prevent the build if a different version of the dependency is available higher in the tree. I understand that it is a simple safe solution but it leads to more confusion than it should.

That's because it could prevent a build when everything is fine.

It sounds like the hoisting mechanism in yarn workspaces should make sure that dependencies that require peer dependencies are placed as close as to the required version of peer dependencies.

Edit

According to this issue - #1795 - the reality is not that simple. It looks like some common dependencies aren't complying with the node module resolution strategy, what is very common in the front end world, or npm makes strange decisions when it comes to placing dependencies.

I have noticed that verifyPackageTree.js - https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/scripts/utils/verifyPackageTree.js#L24 - checks only specific dependencies.

For monorepo users, as a temporary solution, I'd suggest setting a nohoist rule for packages mentioned in the link above.

Edit 2

The nohoist that works for me is:

    "nohoist": [
      "**/babel-eslint",
      "**/babel-jest",
      "**/babel-loader",
      "**/eslint",
      "**/jest",
      "**/webpack",
      "**/webpack-dev-server",
      "**/babel-eslint/**",
      "**/babel-jest/**",
      "**/babel-loader/**",
      "**/eslint/**",
      "**/jest/**",
      "**/webpack/**",
      "**/webpack-dev-server/**"
    ]

Edit 3

The nohoist above caused eslint to stop working completely in some workspaces in my project. Consider it a non-ideal solution.

[GCastilho]

I'm having the same issue, tho using npm and jest

Tree structure like bellow:

root
├── cfg
├── react
│   ├── node_modules
│   │   └── [email protected]
│   ├── package.json
│   ├── package-lock.json
│   ├── public
│   ├── README.md
│   └── src
├── LICENSE
├── node_modules
│   └── [email protected]
├── package.json
├── package-lock.json
├── README.md
├── src
└── tests

So, it seems that react-scripts, for some reason, looks for a jest version in a parent folder, rather than on it's on node_modules sub-folder

For me that makes no sense, why does it have to check for versions higher up the tree? I mean, Is there any reason to check a parent node_modules directory for an package version even when the package was found on its own node_modules folder?

Also: the problem is solved if the server version of jest is 24.8.0, the same react-scripts is using

[lindes]

In this example:

Say you have a package abc that relies on foo and webpack@4, but a package def and ghi that relies on webpack@3.

Why would it be that foo is installed in /node_modules/ instead of /packages/abc/node_modules?

If there's a reason for that that I'm missing, perhaps a more real-world example would be helpful? Otherwise, I fail to understand the argument for "why" the current behavior is intentional.

(This example seems like something that might be worth reporting about, but it would instead be a case of reporting that abc should have its dependency on foo met locally to /packages/abc, or at least warning that that could create problems, rather than simply a more-recent version somewhere up the tree.

And thus, and based on the situations in which I've seen this come up (which are very similar to what's described by nicojs), this currently strikes me as simply being a bug that higher-in-the-tree node_modules directories are checked, once a module is found. However, I remain open to the idea that there's an explanation for why that could be given that would actually be something I hadn't considered.)

[ChrisSargent]

Also running in to this issue in a monorepo using yarn workspaces. Minimal example is here: https://github.com/ChrisSargent/react-scripts-verify-test

You can see that even using the nohoist option for react-scripts (meaning all it's dependencies remain 'local' to the package), it still encounters this issue. FYI, using the nohoist options from this comment #4167 (comment) did not prevent the warnings for me in this minimal repo.

This is the resulting node_modules folder in the package which should lead to correct behaviour.

To me it would make most sense if the verification would follow 'normal' node module resolution rules.

Edit 1:
Actually, I wonder if this is more related to binaries not being hoisted properly as simply running yarn eslint doesn't work (but npx eslint does)

Edit 2:
Actually, with the following no hoist options, yarn eslint runs okay but the pre-flight check still fails.

 "nohoist": [
      "**/react-scripts",
      "**/react-scripts/**"
    ]

[ElvenMonky]

Ok, jest and babel-jest now of version 26. Do we still have to use 24.9.0 because of this?