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

V4 Planned Changes #120

Closed
20 tasks done
bmish opened this issue Jun 13, 2021 · 22 comments
Closed
20 tasks done

V4 Planned Changes #120

bmish opened this issue Jun 13, 2021 · 22 comments
Labels
BREAKING CHANGE This change will require a major version bump

Comments

@bmish
Copy link
Member

bmish commented Jun 13, 2021

I wanted to put together a list of the changes I'm hoping to see in the next major version. What do you think?

@bradzacher
Copy link
Member

I would actually add ALL existing rules. I've used all of this plugin's rules successfully in a number of my eslint plugins, and I find them pretty much all useful and agreeable.

The problem you've got is "pretty much".
The recommended set needs to be unopinionated.
The last thing you want is for someone to disable a rule that's recommended.

Examples of rules that are opinionated: require-meta-type, report-message-format, test-case-property-ordering, test-case-shorthand-strings.

Speaking from experience with @typescript-eslint - you want the recommended set to just be an unopinionated set of rules that catch problems.

@aladdin-add
Copy link
Contributor

there is already a preset so you can extend like:

extends: ["plugin:eslint-plugin/all"]

@bmish
Copy link
Member Author

bmish commented Jun 14, 2021

@bradzacher if the goal is to leave "opinionated" or "stylistic" rules (i.e. about the ordering of properties) out of the recommended config, I'd be fine with that.

Here are the rules I would specifically advocate for being added to the recommended config because they improve rule usability or facilitate eslint plugin development:

Rule Why it could make sense in the recommended config
consistent-output Asserting output is required by ESLint 7 when there's an autofix, and even when there's no autofix, it's still valuable to assert that there's no autofix.
no-deprecated-context-methods We should help people avoid deprecated APIs.
prefer-object-rule The old rule style is deprecated.
prefer-output-null It's significantly easier to distinguish if a test case has an autofix or not when using output: null.
prefer-placeholders See rule doc about benefits.
prefer-replace-text Improved readability.
require-meta-docs-description It's easier for users to understand rules when they have descriptions. But this rule currently requires descriptions to begin with enforce, require, or disallow which might make it too opinionated for the recommended config if people often use other description formats, so we could enable it with pattern: '.+' to allow any description.
require-meta-docs-url It's easier for users to understand rules when they provide a link to their documentation.
require-meta-has-suggestions This new property will be required for suggestable rules in ESLint 8.
require-meta-schema Enforcing rule schemas reduces the risk of users configuring a rule incorrectly, and even rules that have no options should provide schema: [] so that eslint can validate that no options are passed to them.
require-meta-type This enables users to choose what type of rule they want to run (based on the three types used by eslint core: problem, suggestion, or layout). However, if users commonly use types other than these, then I agree this rule could be considered too opinionated to include in the recommended config.

Here are rules which could be considered too opinionated/stylistic to include in the recommended config.

Rule Why it may be too opinionated / stylistic to include in the recommended config
meta-property-ordering Enforces an opinionated ordering of properties.
prefer-message-format Enforces an opinionated (albeit pretty minimal) format (starts with capital letter, ends of period) for error messages.
test-case-property-ordering Enforces an opinionated ordering of properties.
test-case-shorthand-strings Enforces using a string instead of object to make basic test cases more concise. This could be considered an opinionated/stylistic preference at odds with test case consistency.

@aladdin-add
Copy link
Contributor

agreed with @bradzacher .

eslint has set a very high bar to eslint:recommended (eslint/eslint#14673). I think better to adopt the similar policy in the plugin. I'm 👍 to the following rules:

  • consistent-output
  • no-deprecated-context-methods
  • prefer-object-rule
  • require-meta-has-suggestions
  • require-meta-type

@bmish
Copy link
Member Author

bmish commented Jun 15, 2021

Sounds good. Can I ask why you would exclude these ones:

  • require-meta-docs-url
  • require-meta-schema

I consider these two especially high impact in that they both improve external-facing rule usability by helping developers understand lint rules (by providing an IDE documentation link) and ensuring developers configure rules correctly.

@aladdin-add
Copy link
Contributor

yes, it is a best practice, and should be enabled in most cases, but there are some exceptions, e.g. internal-rules: https://github.com/eslint/eslint/tree/master/tools/internal-rules

@bradzacher
Copy link
Member

bradzacher commented Jun 15, 2021

Example:
None of the ~400 internal rules at Facebook have a schema defined, because they take no options.
Nor do they have a docs url defined, because we have our own infra for dealing with rule docs based solely on the rule ID.

For the wider world - docs url assumes the author has written and maintained docs (not always true).

And yeah whilst technically it's best if you define a schema even without options. It's certainly not a huge deal if you don't when you have no options.

@bmish
Copy link
Member Author

bmish commented Jun 15, 2021

Having no rule options is totally fine. The require-meta-schema rule doc explains that schema: [] should be specified for rules that do not have options in order to enforce that no options are accidentally passed to such rules, and rules that specify schema: [] are allowed by this rule.

@bmish
Copy link
Member Author

bmish commented Jun 15, 2021

Thanks to both of you for bringing up some use cases where a plugin might not want to specify rule URLs. Those are helpful.

@bmish
Copy link
Member Author

bmish commented Jul 9, 2021

I have an update to share regarding the require-meta-schema rule. I have recommended to the ESLint team that schemas should be required for rules with options and they are interested in this idea so I'll be opening an RFC for it targeting ESLint 9.

With this change, rules without options will no longer need to specify schema: [], as that will be the default behavior. That means we can update the require-meta-schema rule to only require the schema property for rules that actually have options (which is the same as how require-meta-fixable only requires the fixable property for rules with fixers).

As a result of this change, we should be able to enable the require-meta-schema rule as recommended in eslint-plugin-eslint-plugin v4.

@bmish bmish mentioned this issue Sep 21, 2021
3 tasks
@aladdin-add
Copy link
Contributor

As eslint v8.0.0 will be released in a few weeks, we can start merging these PRs, and made a prerelease version. thoughts? @bmish

@bmish
Copy link
Member Author

bmish commented Sep 23, 2021

@aladdin-add I believe all PRs for the next release are ready, except for #179 which I can try to get to soon. A prerelease version is a good idea.

@bmish
Copy link
Member Author

bmish commented Sep 23, 2021

Two more notes:

  1. We should release a minor version first with these two non-breaking changes: Fix: Remove erroneous schema from require-meta-schema rule #178 and New: Add requireSchemaPropertyWhenOptionless option to require-meta-schema rule #180.
  2. We should also consider including TypeScript support (TS support #176) in this release. TS support will build on top of ESM rule support (Breaking: Support ESM rules #177) and I could give it a try, but Breaking: Support ESM rules #177 needs to be merged first for me to do so.

@bmish
Copy link
Member Author

bmish commented Sep 24, 2021

Open question: I have added a requireSchemaPropertyWhenOptionless option (default true to maintain current behavior) to require-meta-schema to address the above concerns. We have a few paths going forward:

  1. Leave option enabled for v4 release. Disable it once ESLint changes their default behavior (possibly in ESLint v9) to require schemas for rules with options (since then it will no longer be necessary to specify schema: [] for rules without options).
  2. Disable the option in v4 release.

@aladdin-add
Copy link
Contributor

re requireSchemaPropertyWhenOptionless: I'm fine either way. which one do you prefer?

@bmish
Copy link
Member Author

bmish commented Sep 24, 2021

I personally think we should enforce the more strict behavior (keep requireSchemaPropertyWhenOptionless enabled by default on require-meta-schema) for now until it becomes no longer necessary (as a result of a potential future ESLint change).

@bmish
Copy link
Member Author

bmish commented Sep 25, 2021

I realized that a lot of our existing rules need to be updated to handle suggestions. I think it's pretty important that we correct this gap in coverage in this major release, as the suggestions feature has been available for nearly 2 years now. I've opened issues to track these gaps, and I will try to open PRs to address these in the coming days. The changes required should not be that large.

In the meantime, we should continue to work on getting all the other changes for the major release merged.

@aladdin-add
Copy link
Contributor

👍 nice catch!

@bmish
Copy link
Member Author

bmish commented Oct 11, 2021

@aladdin-add thanks for releasing the prerelease version. I'll spend a few days now testing it and checking for false positives/negatives before the final release.

@aladdin-add
Copy link
Contributor

@bmish I've tested it in a few plugins - all working fine, an excellent job! 🎉

just pushed 4.0.0 - let's wait and see if need patch releases. 😄

@SimenB
Copy link

SimenB commented Oct 15, 2021

🎉

I opened up a couple of bug reports (#214, #215). Not reduced, can do if needed (won't be until next week, tho)

@bmish
Copy link
Member Author

bmish commented Oct 15, 2021

Release completed! https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/releases/tag/v4.0.0

Will address any bugs separately.

@bmish bmish closed this as completed Oct 15, 2021
@bmish bmish added the BREAKING CHANGE This change will require a major version bump label Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump
Projects
None yet
Development

No branches or pull requests

4 participants