Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peer dependencies of eslint-config-react-app should probably be regular dependencies #6129

Closed
kirill-konshin opened this issue Jan 5, 2019 · 16 comments
Assignees

Comments

@kirill-konshin
Copy link

kirill-konshin commented Jan 5, 2019

If user wants only the config and nothing else the one should still install all the deps, which can be done automatically and with no hassle.

Currently, I have to install eslint-plugin-flowtype which I am not even using in my project...

Moreover, version of babel-eslint is outdated (available: 10, listed: 9).

As one of the solutions, maybe, we can create one extra package in this repo and name it like eslint-config-react-app-pack, which will have all the needed deps?

@mrmckeb
Copy link
Contributor

mrmckeb commented Jan 6, 2019

Hi @kirill-konshin, I understand what you're saying, and in a way I agree with you - but I also think there are a lot of strengths in the peer-dependency approach. For one, it helps to eliminate issues where multiple versions of the same dependency are installed - which can cause problems.

The obvious problem with peer dependencies, which I've faced too, is that they can be outdated at times and even if the new version is compatible, you keep seeing errors upon install (which is annoying). The other issue which you've raised above is the optional dependency issue - this isn't supported by npm.

Perhaps the better approach is for us to publish a base config, and then a flow config that extends the base config - then you won't hit this issue.

What are your thoughts on that?

@mrmckeb mrmckeb self-assigned this Jan 6, 2019
@kirill-konshin
Copy link
Author

I agree, it does makes sense to divide packages (or configs) into a more granular set.

@stale
Copy link

stale bot commented Feb 7, 2019

This issue 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 stale bot added the stale label Feb 7, 2019
@kirill-konshin
Copy link
Author

Anti-stale-bot-ping )

@stale
Copy link

stale bot commented Mar 9, 2019

This issue 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 stale bot added the stale label Mar 9, 2019
@kirill-konshin
Copy link
Author

Anti stale bot ping

@stale stale bot removed the stale label Mar 9, 2019
@skoestler
Copy link

I have a project that is just a component library so I'm not using create-react-app but I wanted the tooling to be consistent with our CRA projects so using babel-preset-react-app and eslint-config-react-app is a great option (I really appreciate that they are published as they are).

I did find it odd that babel-preset-react-app includes all of the plugins as dependencies and eslint-config-react-app uses peer dependencies.

I admit that I could totally be missing the reason for why they are different, but in my case this change would be helpful.

@stale
Copy link

stale bot commented Apr 9, 2019

This issue 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 stale bot added the stale label Apr 9, 2019
@stale stale bot removed the stale label Apr 9, 2019
@tlrobinson
Copy link
Contributor

I agree with @kirill-konshin and @skoestler that eslint-config-react-app should mirror the behavior of babel-preset-react-app by including its dependencies as regular dependencies.

I set up a project that mirrors create-react-app's Babel and ESLint config and about half of the dependencies in my package.json are eslint-config-react-app's dependencies. It would be nice to trim that down so that one could just add eslint-config-react-app without having to track down all of the dependencies.

@goodwin64
Copy link

Hi!
Just faced with the same issue. Little more details:
I have the following project structure:

namespace-project
  | subpackage-project-1
  | subpackage-project-2
  | subpackage-project-N

Where namespace-project is a Lerna root and subpackage-project is either a local lib or an app built with CRA.

After I run npm run start (react-scripts start) the console is clear and it says that the app is running:

Starting the development server...
Files successfully emitted, waiting for typecheck results...
Compiled successfully!

You can now view @namespace/subpackage-project-1 in the browser.

  Local:            http://localhost:3000
  On Your Network:  http://***.***.*.***:3000

BUT after I make some changes and hot-reload updates the app, I see the following warnings in the console:

Failed to load plugin 'flowtype' declared in 'BaseConfig » C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-config-react-app\index.js': Cannot find module 'eslint/lib/rules/no-unused-expressions'
Require stack:
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-plugin-flowtype\dist\rules\noUnusedExpressions.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-plugin-flowtype\dist\index.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\eslint\lib\cli-engine\config-array-factory.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\eslint\lib\cli-engine\cascading-config-array-factory.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\eslint\lib\cli-engine\cli-engine.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\eslint\lib\cli-engine\index.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\eslint\lib\api.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-loader\dist\getOptions.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-loader\dist\index.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\eslint-loader\dist\cjs.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\loader-runner\lib\loadLoader.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\loader-runner\lib\LoaderRunner.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\webpack\lib\NormalModule.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\webpack\lib\NormalModuleFactory.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\webpack\lib\Compiler.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\node_modules\webpack\lib\webpack.js
- C:\Users\goodwin64\WebstormProjects\namespace-project\packages\subpackage-project\node_modules\react-scripts\scripts\start.js

Search for the keywords to learn more about each warning.
To ignore, add // eslint-disable-next-line to the line before.
  1. What can I do to resolve it? Shall I install the flowtype as a devDependency in all my subpackage projects?
  2. Are there some other eslint plugins that are also required?

@tobiaskraus
Copy link

In react-scripts/package.json (react-scripts@3.4.1), all the eslint plugins are now as dependencies (not peerDependencies)

  "dependencies": {
    ....
    "eslint": "^6.6.0",
    "eslint-config-react-app": "^5.2.1",
    "eslint-loader": "3.0.3",
    "eslint-plugin-flowtype": "4.6.0",
    "eslint-plugin-import": "2.20.1",
    "eslint-plugin-jsx-a11y": "6.2.3",
    "eslint-plugin-react": "7.19.0",
    "eslint-plugin-react-hooks": "^1.6.1",

Can this issue be closed now?

I found this issue, because in my created issue #9083 i suggest something different: to be able to use own installed versions of eslint-plugins. But probably this doesn't mean, that they cannot be defined as regular dependencies when being able to resolve them with an .env flag from root directory in webpack config (see proposal of #9083)

@mrmckeb
Copy link
Contributor

mrmckeb commented May 29, 2020

This is related to the discussion here, so I think we can close this - thanks @tobiaskraus for the reminder.

#9069

@mrmckeb mrmckeb closed this as completed May 29, 2020
@kirill-konshin
Copy link
Author

Why is this closed? Package.json of eslint-config-react-app is still having peerDependencies: https://github.com/facebook/create-react-app/blob/master/packages/eslint-config-react-app/package.json#L17, it is still an issue.

@mrmckeb
Copy link
Contributor

mrmckeb commented May 31, 2020

@kirill-konshin please see the discussion I linked to.

@kirill-konshin
Copy link
Author

@mrmckeb yes, I'm well aware of the issues with shared configs and ESLint, especially with Yarn 2 with PNP mode on: eslint/eslint#3458. The issue has to remain open because at some point ESLint will be fixed.

@mrmckeb
Copy link
Contributor

mrmckeb commented Jun 3, 2020

@kirill-konshin when the root problem is fixed, we'll definitely make the required changes. All of the team are tracking this.

As this issue stands, it's not possible to resolve, and leaving it open creates confusion and noise for the volunteers working on this project.

@lock lock bot locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants