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

Handle deprecated rules #112

Closed
alexilyaev opened this issue Oct 5, 2019 · 15 comments
Closed

Handle deprecated rules #112

alexilyaev opened this issue Oct 5, 2019 · 15 comments

Comments

@alexilyaev
Copy link
Contributor

alexilyaev commented Oct 5, 2019

Not sure what's the policy here for deprecated rules.
My use case is to catch deprecated rules in an existing config using eslint-find-rules.
The issue is that projects have configurations with deprecated rules and ESLint does not warn about it.

So I'd like to catch such cases, possible with:

npx eslint-find-rules -d .eslintrc

Ideally this would exit with a 0 value and when I bump ESLint or related configs/plugins, it'll exit with non 0 and show me which rules are deprecated.

Right now it's not possible when extending eslint-config-prettier:

.eslintrc:

{
  "extends": ["prettier"]
}
$ npx eslint-find-rules -d .eslintrc

deprecated rules

indent-legacy    no-spaced-func
@alexilyaev alexilyaev changed the title react/jsx-space-before-closing has been deprecated Deprecated rules Oct 5, 2019
@lydell
Copy link
Member

lydell commented Oct 6, 2019

Hi!

There used to be no deprecated rules included in this config, but then they were added after #8 was filed.

@alexilyaev
Copy link
Contributor Author

@lydell Yeah, I've read it.

Can you share your thoughts on the use case I've mentioned of validating user configured rules don't use deprecated ones?

Maybe we can add them conditionally based on the version of ESLint the user is using?

@lydell
Copy link
Member

lydell commented Oct 6, 2019

One idea I have is to move them to a separate, opt-in file. So if you need it, you can do:

{
  "extends": [
	"prettier",
    "prettier/legacy",
	"prettier/react",
    "prettier/react-legacy"
  ]
}

I wonder who needs the legacy rules to be turned off. Is it advanced users with a legacy code base? Then it might be fine to split. Is it beginners? Then they might be confused. But on the other hand – wouldn’t beginners start out with new stuff? I wonder what configs still include those legacy rules.

Another thought – could the eslint-find-rules tool be smarter about this somehow maybe? Perhaps it could ignore deprecated rules that come from third-party configs. Or maybe it could ignore deprecated rules that are set to "off".

Not sure what the best solution is yet. Do you have any more ideas?

Maybe we can add them conditionally based on the version of ESLint the user is using?

I don’t think that’s possible in a config package.


Just thought of a possible workaround. Instead of extending "prettier" directly, you could extend a local file.

{
  "extends": ["./prettier-workaround"]
}

And define prettier-workaround.js like so:

const prettierConfig = require("eslint-config-prettier");

const { "no-spaced-func": _1, "indent-legacy": _2, ...rules } = prettierConfig.rules;

module.exports = {
  rules,
};

@alexilyaev alexilyaev changed the title Deprecated rules Handle deprecated rules Oct 8, 2019
@alexilyaev
Copy link
Contributor Author

@lydell Thanks for giving it some attention :-).

  1. Splitting to legacy would be adding complexity and confusion to users, which is definitely undesirable. Anything we can do to make things work out-of-the-box is best.

  2. The deprecated rules here only make sense on older versions of ESLint or plugins.

  • no-spaced-func can be removed safely since we have peerDependencies: "eslint": ">=3.14.1".
  • indent-legacy is not deprecated for users of eslint <4, so here we should handle it somehow.
  • react/jsx-space-before-closing we don't have it in peerDependencies as it is optional, and there's no such thing as optionalPeerDependencies, so again, we should handle somehow.
  1. I dug into eslint-find-rules and it's not so simple but there might be a way:
    Report deprecated rules that are in use sarbbottam/eslint-find-rules#188 (comment)

  2. Why can't eslint-config-prettier access the user's package.json and check the installed versions of eslint and plugins?

e.g.

const path = require('path');
const packageJsonPath = path.resolve(process.cwd(), 'package.json');
const packageJson = require(packageJsonPath);
  1. Sure I can work around it for my specific needs but I wanted to have a solution for others as well.

@lydell
Copy link
Member

lydell commented Oct 9, 2019

I somehow forgot that the configs exported from this package are JS files! So your idea of running some code before setting module.exports might actually work.

@alexilyaev
Copy link
Contributor Author

Working on a POC, works for now.

@lydell
Copy link
Member

lydell commented Oct 18, 2019

@alexilyaev I just had an idea – we could look for a certain environment variable. If it’s set, don’t include deprecated rules. What do you think about that?

ESLINT_CONFIG_PRETTIER_EXCLUDE_DEPRECATED=true npx eslint-find-rules -d .eslintrc

@alexilyaev
Copy link
Contributor Author

alexilyaev commented Oct 18, 2019

Hmm, that's an option, but requires an extra step from the user of eslint-find-rules.
Ideally this would be handled automatically.

I already have a working POC, it does the following...

  • Check the installed eslint package version
    • If not installed, check the dependencies/devDependencies in package.json for the version (TBD)
  • Conditionally load the problematic rules based on the version they were deprecated, comparing versions with semver package.

Looks something like this:

module.exports = {
  rules: {
    ...
    "generator-star-spacing": "off",
    "implicit-arrow-linebreak": "off",
    "indent": "off",
+    ...conditionalRule(semver.lt(eslintVersion, "4.0.0"), {
+      "indent-legacy": "off"
+    }),
    "jsx-quotes": "off",
    "key-spacing": "off",
    ...
  }
}

There are other ways to do this, but less clean.
Just that the ... Object spread is supported only from node >v8.10:
https://kangax.github.io/compat-table/es2016plus/#test-object_rest/spread_properties_object_spread_properties

We depend on "eslint": ">=3.14.1" and that version of eslint depends on "node": ">=4".
What should be the minimal node version that we support?
Granted the configs right now are just objects so any node version will do, but the CLI is written in ES6.

@lydell
Copy link
Member

lydell commented Oct 18, 2019

I think your solution sounds nice, but maybe a little overkill? I’m also worried that this will cause issues for people since resolving packages is always harder than you first thought, in my experience.

@lydell
Copy link
Member

lydell commented Oct 18, 2019

Another idea: Maybe way too hacky, but maybe we could look att process.argv and check that it does not contain "eslint-find-rules"...

@alexilyaev
Copy link
Contributor Author

I think your solution sounds nice, but maybe a little overkill? I’m also worried that this will cause issues for people since resolving packages is always than you first though, in my experience.

Not sure what that means...
"resolving packages is always than you first though"
You mean "harder than you first think"?

Here's how I'm doing it, adding lib/read-module-pkg.js:

const path = require("path");
const readPkg = require("read-pkg");

module.exports = moduleName => {
  let modulePkg = null;

  try {
    modulePkg = readPkg.sync({
      normalize: false,
      cwd: path.resolve(process.cwd(), "node_modules", moduleName)
    });
  } catch (error) {
    // Ignore the error if failed
  }

  return modulePkg;
};

And then outside...

const readModulePkg = require("./lib/read-module-pkg");

const eslintDepPkg = readModulePkg("eslint");
const eslintVersion = eslintDepPkg && eslintDepPkg.version;

@alexilyaev
Copy link
Contributor Author

Another idea: Maybe way too hacky, but maybe we could look att process.argv and check that it does not contain "eslint-find-rules"...

eslint-find-rules can be executed in other ways as well (e.g. node script, terminal alias), so it won't cover all cases.

@alexilyaev
Copy link
Contributor Author

Let me PR soon and we'll talk.

@alexilyaev
Copy link
Contributor Author

My initial solution didn't cut it #120 (comment)
Eventually settled on a much simpler approach #121

@lydell
Copy link
Member

lydell commented Oct 26, 2019

Released in v6.5.0. Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants