Eslint-plugin-react: Warning: React version not specified when used with monorepo

[jagretz]

I looked through the code for version.js and the version test so I believe this is a valid issue.

I am running eslint from a monorepo I've set up with lerna. One of the packages in the monorepo using react so I've only installed react-specific eslint plugins in that package. ie eslint-plugin-react, eslint-plugin-react-hooks etc

Using eslints overrides feature and running an eslint command from the _root_ (./) works picking up and overriding the root configuration when it finds a eslintrc in a sub package, however, when I've set my react version to detect from the react projects .eslintrc it throws this warning.

Warning: React version was set to "detect" in eslint-plugin-react settings, but the "react" package is not installed. Assuming latest React version for linting.

react _is_ installed and included in _that_ packages /react-foo/package.json. But in order to prevent this warning I would need to install react at the root package of the monorepo which isn't correct or desired.

Here is an example project hierarchy with the .eslintrc config overrides:

./
├── package.json            # react is not installed in this package
├── .eslintrc               # root lint config
├── packages/
|   └── react-foo/
|   |   ├── package.json    # react installed
|   |   ├── .eslintrc       # react-specific lint overrides
|   |   └── src/
|   |       └──  ...
|   └── not-react-foo/
|       ├── package.json
|       └── src/
|           └── ...
└── ...

I believe this is due to this tools version.js using process.cwd() to check for react at the (cwd) directory where the command was ran, ie the root (./.eslintrc), instead of directory from where the eslint configuration for eslint-plugin-react is specified, ie ./packages/react-foo/.eslintrc.

[ljharb]

It needs to be a sibling to the root .eslintrc, so yes, i'd say it is correct to have react located in a root node_modules. Typically in a monorepo, all deps are hoisted to the root and symlinked down - why is this not the case here?

[ljharb]

(related to #2214)

[ljharb]

hmm, that's a good point tho - maybe the detection should start looking from the location of the file being linted, not from the cwd? That seems a reasonable change.

[jagretz]

I just started using lerna so I don't have any sort of recommended "best practices" for how to use the tool most effectively. From my understanding, while symlinking packages ("hoisting") could make other tooling run more consistently, I don't think that makes the most sense for dependencies that aren't _shared_ across sub packages.

Lerna states hoist is for _shared_ dependencies but maybe in time I'll learn it's simply easier to hoist everything. I think this particular issue is actually mentioned in lerna's documentation on hoisting under Disadvantages with hoisting > Module resolution.

If you don't mind making the change that would be great. I could also look into it this weekend, though my knowledge of node APIs is sub-par at best. Only one way to learn though 🤪. Let me know if you want to take care of it yourself or if you would be interested in a contribution. Cheers

[ljharb]

A PR would be great, thanks.

[jasonwilliams]

hmm, that's a good point tho - maybe the detection should start looking from the location of the file being linted, not from the cwd? That seems a reasonable change.

has an issue been raised for this specific issue? It took me a while to realise this was the problem, if not i can raise one

[ljharb]

@jasonwilliams i think this issue is that issue. It just needs a PR at this point.

[migueloller]

@ljharb, I'm running into this issue as well as the React at the root of the monorepo is an outdated but the non-hoisted ones are the ones that need checking. I'm open to adding a PR if it's something nobody else is working and if you or any other maintainer wouldn't mind giving me some pointers as where to start.

[ljharb]

Sure, a PR would be great. I think the first thing would be to craft failing test cases for the behavior you want.

[migueloller]

I'm playing around with the tests and the code here: https://github.com/yannickcr/eslint-plugin-react/blob/80f3826dd854796d25244ad34782b9147b90db1d/lib/util/version.js#L19

The issue I'm seeing is that while there are unit tests, and fixtures set up with a dummy node_modules and React, the context object being passed in to testReactVersion and testFlowVersion from the tests is not the full ESLint context.

The way I would go about implementing this is by using context.getFilename() and changing that line of code to use the file's path instead of process.cwd(). At the very least, it should use context.getCwd() instead of process.cwd().

Anyhow, it's not clear to me how to bet set this up in the project. Are there currently any integration tests with ESLint or is it just unit tests?

[ljharb]

Just unit tests; eslint provides a RuleTester for this purpose.

[migueloller]

@ljharb, thanks for pointing that out. I'll take another look when I get a chance and see how I can use RuleTester to test the new implementation.