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

Keep eslint-config-react-app's dependencies up-to-date. #3217

Closed
jacksonrayhamilton opened this issue Sep 29, 2017 · 16 comments
Closed

Keep eslint-config-react-app's dependencies up-to-date. #3217

jacksonrayhamilton opened this issue Sep 29, 2017 · 16 comments

Comments

@jacksonrayhamilton
Copy link

Continuing on the issue brought up here: #2631

I think eslint-config-react-app would be easier to use if its peer dependencies were up-to-date with their packages' latest breaking changes. This just happened to me:

  • Tried to lint a package extending "react-app"
  • ESLint complains that I don't have "react-app" installed
  • Installed it with the usual npm i -g eslint-config-react-app
  • ESLint complains again about the missing peer dependencies, which NPM in all its wisdom and usefulness won't install for us automatically
  • Supposedly install them all with the usual npm i -g eslint-plugin-flowtype eslint-plugin-import eslint-plugin-jsx-a11y eslint-plugin-react babel-eslint
  • See weird error, "Definition for rule 'jsx-a11y/href-no-hash' was not found"
  • Google and finally find these specific versions I need to install: https://github.com/facebookincubator/create-react-app/tree/master/packages/eslint-config-react-app#usage-outside-of-create-react-app

It'd be nice if the latest versions just worked, since I typically install the latest versions of packages. and I imagine other people do, too.

@Timer
Copy link
Contributor

Timer commented Sep 29, 2017

This was a breaking change so we cannot just "support" it, and our documentation outlines the exact versions of the packages you need to install.

Soon, I think we'll be able to actually depend on our current peer deps within our preset, but it's a newer (if available now?) eslint feature.

@jacksonrayhamilton
Copy link
Author

This was a breaking change so we cannot just "support" it, and our documentation outlines the exact versions of the packages you need to install.

What I'm suggesting is to make a concerted effort to keep peer dependencies up-to-date. I'm suggesting how to make the usage of this software more intuitive; providing a way to avoid the need to look up how to use it.

@jacksonrayhamilton
Copy link
Author

jacksonrayhamilton commented Sep 29, 2017

Soon, I think we'll be able to actually depend on our current peer deps within our preset, but it's a newer (if available now?) eslint feature.

I believe this is the issue we might want to track for that feature: eslint/eslint#6237 (comment)

EDIT: Or this one: eslint/eslint#3458

@Timer
Copy link
Contributor

Timer commented Sep 29, 2017

I'm open to a PR that updates our peer dep range to accept both versions, but you'll need to fix the configuration as well to toggle what's being used based on the peer dep'd version.

@jacksonrayhamilton
Copy link
Author

I think this is what you're suggesting?

diff --git a/packages/eslint-config-react-app/index.js b/packages/eslint-config-react-app/index.js
index adcdb86..727446d 100644
--- a/packages/eslint-config-react-app/index.js
+++ b/packages/eslint-config-react-app/index.js
@@ -277,7 +277,6 @@ module.exports = {
     'jsx-a11y/aria-role': 'warn',
     'jsx-a11y/aria-unsupported-elements': 'warn',
     'jsx-a11y/heading-has-content': 'warn',
-    'jsx-a11y/href-no-hash': 'warn',
     'jsx-a11y/iframe-has-title': 'warn',
     'jsx-a11y/img-redundant-alt': 'warn',
     'jsx-a11y/no-access-key': 'warn',
@@ -293,3 +292,23 @@ module.exports = {
     'flowtype/use-flow-type': 'warn',
   },
 };
+
+try {
+  var a11y = require('eslint-plugin-jsx-a11y');
+} catch (e) {
+  // Ignore if the package isn't installed, let ESLint complain about that.
+}
+if (a11y) {
+  // Feature-detect availability of newly-added / later-deprecated rules, since
+  // we are maintaining backwards-compatibility for version 5 and ease-of-use
+  // for version 6.
+  if (a11y.rules['anchor-is-valid']) {
+    Object.assign(module.exports.rules, {
+      'jsx-a11y/anchor-is-valid': 'warn',
+    });
+  } else if (a11y.rules['href-no-hash']) {
+    Object.assign(module.exports.rules, {
+      'jsx-a11y/href-no-hash': 'warn',
+    });
+  }
+}
diff --git a/packages/eslint-config-react-app/package.json b/packages/eslint-config-react-app/package.json
index a10ef67..c3f16bb 100644
--- a/packages/eslint-config-react-app/package.json
+++ b/packages/eslint-config-react-app/package.json
@@ -15,7 +15,7 @@
     "eslint": "^4.1.1",
     "eslint-plugin-flowtype": "^2.34.1",
     "eslint-plugin-import": "^2.6.0",
-    "eslint-plugin-jsx-a11y": "^5.1.1",
+    "eslint-plugin-jsx-a11y": "^5.1.1 || ^6.0.2",
     "eslint-plugin-react": "^7.1.0"
   }
 }

It seems to me like this should work, but I get "Error: Cannot find module 'eslint-plugin-jsx-a11y'". Maybe we need to imitate the way ESLint loads plugins? But in their lib/config/plugins.js, it looks like they use require. Any ideas?

@Timer
Copy link
Contributor

Timer commented Sep 29, 2017

Yup, that's what I had in mind in terms of compatibility with both versions.

No ideas on the missing package though, node's module resolution should be picking it up -- odd.

Does this work when you use npm run create-react-app ~/Desktop/test?

@karl-run
Copy link

I'm having issues with this now because we are using newest eslint-config-airbnb which has a peer dep of eslint-plugin-jsx-a11y@^6.0.2. Because eslint-config-react-app requires eslint-plugin-jsx-a11y@^5.1.1 I get a bunch of

[1] Line 1: Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash

on every build.

@gaearon
Copy link
Contributor

gaearon commented Oct 31, 2017

It is not supported to use Airbnb config with CRA project without ejecting. In fact it's not supported to use any extra config at all until you eject.

We'll bump the version eventually, but the use case you rely on has never been supported in the first place.

@karl-run
Copy link

It might not be supported, but it works fine as long there's no mismatch in major peer dependencies.

@gaearon
Copy link
Contributor

gaearon commented Oct 31, 2017

Yeah. I’m just saying that while we understand it’s an inconvenience, it’s pretty much expected that versions will fall out of sync if you use it this way. So we’ll fix it within a month or two but it‘s not high on our list.

@gaearon
Copy link
Contributor

gaearon commented Jan 8, 2018

I'll close as this won't be our priority. We try to update deps when we can, but supporting multiple majors is unlikely to happen.

@gaearon gaearon closed this as completed Jan 8, 2018
@Tobbe
Copy link

Tobbe commented Jan 16, 2018

I just got bit by the exact same thing myself :(

npm WARN eslint-config-airbnb@16.1.0 requires a peer of eslint-plugin-jsx-a11y@^6.0.2 but none is installed. You must install peer dependencies yourself.

npm WARN eslint-config-react-app@2.1.0 requires a peer of eslint-plugin-jsx-a11y@^5.1.1 but none is installed. You must install peer dependencies yourself.

Please reconsider updating to v6 of eslint-plugin-jsx-a11y

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

@Tobbe

We already updated it on the next branch but we can't cut a release because it's just not ready. If you insist on using two different configs (our default one and Airbnb's) at the same time, it is up to you to figure out what to do with their dependencies. We can't possibly be compatible with all presets at all times.

@Tobbe
Copy link

Tobbe commented Jan 16, 2018

Of course you can't. Fully understandable :) But thanks for pointing me to @next, I'll see about using that for now.

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2018

It's not usable yet (it only exists on GH, and next on npm is the old one), but we hope to publish it within a week or two. You can track this in #3815.

@mmazzarolo
Copy link

Hey @gaearon, just to make sure I understood correctly: the eslint-plugin-react-app package release cycle is strictly tied to the react-scripts one, am I right?

Just asking because I'm maintaining eslint-plugin-react-app and this info might be useful.

Thanks!

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

No branches or pull requests

6 participants