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

Change default rule schema to schema: [] and drop support for function-style rules #14709

Closed
bmish opened this issue Jun 15, 2021 · 19 comments · Fixed by #17792
Closed

Change default rule schema to schema: [] and drop support for function-style rules #14709

bmish opened this issue Jun 15, 2021 · 19 comments · Fixed by #17792
Assignees
Labels
breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@bmish
Copy link
Sponsor Member

bmish commented Jun 15, 2021

The problem you want to solve.

Today, rule schemas (meta.schema) are optional. This has the following effects:

  • If a rule does not specify a schema, nothing is enforced.
  • Rules with options may not have a schema specified, despite it being best practice to specify a schema for rule options. This increases the chances of developers passing invalid rule options and encountering unexpected behavior / bugs in rules.
  • If a rule author wants to enforce that no options are passed to their rule, they have to manually specify schema: [], otherwise it's possible that developers could accidentally provide useless rule options to their rule without knowing it.

There is an existing, third-party lint rule eslint-plugin/require-meta-schema to enforce that rules have schemas, but this rule is optional and thus not widely used.

Your take on the correct solution to problem.

Change the default rule schema to schema: [] to enforce that no rule options are passed to rules that do not specify a schema.

This has the desirable consequence of requiring rules with options to begin specifying their schema if they did not already.

This would be a breaking change suitable for the next major release.

Making ESLint's default behavior more strict like this goes along with many recent changes to tighten validation (i.e. more RuleTester class validation in ESLint 7, increased rule configuration validation in ESLint 6, etc) to improve rule usability and reduce the chance of mistakes.

And in addition to the obvious benefits of increased rule schema usage such as reduced chance of silent mistakes/bugs, being able to rely on having rule schemas available could enable:

  • Improved/autogenerated documentation regarding rule options
  • TypeScript-style autocomplete/IntelliSense when configuring a rule

Are you willing to submit a pull request to implement this change?

Yes. I am open to writing the RFC and implementation.

@bmish bmish added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 15, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 15, 2021
@nzakas nzakas added breaking This change is backwards-incompatible and removed triage An ESLint team member will look at this issue soon labels Jun 16, 2021
@nzakas
Copy link
Member

nzakas commented Jun 16, 2021

Interesting idea. I think we kept this as-is for maximum compatibility with older rules that were implemented before rules could specify schemas. The big question we need to answer is how many existing plugins would break.

Are you thinking this would apply just to object-based rules and function-based rules would still be schemaless?

I think it’s an idea worth exploring, though as a breaking change, it would need to wait until v9.0.0, as we have frozen the feature set for v8.0.0.

Let’s see what others think.

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Jun 16, 2021
@bmish
Copy link
Sponsor Member Author

bmish commented Jun 16, 2021

Targeting v9 should give us ample time (I assume a year) to communicate about this upcoming breaking change and help rules prepare for it. Other ways we could help rules prepare:

  1. When running rule tests, ESLint could show a warning when the test case specifies options but the rule has no schema:

    WARNING: Rules with options will be required to specify meta.schema in ESLint v9.

  2. eslint-plugin/require-meta-schema could be made into a recommended rule in that plugin.
  3. Encourage greater adoption of eslint-plugin-eslint-plugin by publicizing it in ESLint plugin documentation / blog posts.

Regarding the question of whether this requirement should apply to both object-based rules and function-based rules, I'm not sure yet. Ideally, it would apply to all rules, but I'm open to excluding function-based rules if there's a strong case made for that. Will function-based rules be supported forever, or can we remove support for that deprecated style at some point?

@nzakas
Copy link
Member

nzakas commented Jun 17, 2021

We are slowly making creating function rules harder, for instance, in v8.0.0 they can no longer be fixable (#13349) and can’t provide suggestions. So it’s not outside the realm of possibility that we could remove support completely.

@btmills
Copy link
Member

btmills commented Jun 27, 2021

I'm also intrigued by this. I'd feel better if we had some data about how many rules currently don't provide schemas because by doing this, we'd also be compelling them to make a breaking change.

@bmish
Copy link
Sponsor Member Author

bmish commented Jun 28, 2021

With some effort, we/I could write a script to download the ~3,500 eslint plugins (titled eslint-plugin-* or keyword:eslint-plugin,eslintplugin) from NPM or github (hopefully without hitting rate-limiting), then check files in lib/rules or src/rules to calculate what percentage of rules mention context.options but not schema (which would indicate they need to add a schema).

Does that sound reasonable, or are there any better/easier ideas for collecting the data? And is there any specific finding you would want to see to feel comfortable moving forward?

Regardless of what any analysis finds, I do believe that over the next year, we could help plugins prepare for this change with the ideas I outlined in my previous comment.

And we could use this same approach in order to move forward with removing support for other deprecations like function-based rules (the lint rule eslint-plugin/prefer-object-rule would help).

@nzakas
Copy link
Member

nzakas commented Jun 29, 2021

@bmish that sounds like a great approach. If you have the time to do that, we would appreciate it.

@btmills
Copy link
Member

btmills commented Jun 29, 2021

@bmish having that data available would be amazing as we make the sorts of decisions you mentioned! Please share the code if you’re able because I could see us running it periodically to answer new questions that come up.

@bmish
Copy link
Sponsor Member Author

bmish commented Jul 6, 2021

I put together a repository called eslint-ecosystem-analyzer with scripts to perform an analysis of the ESLint plugin ecosystem. It retrieves the top ESLint plugin repositories by searching GitHub and then clones them so that metrics can be computed.

The analysis output is included below. To summarize the findings:

  • For the top 100 ESLint plugins, 1% of total rules have options but are missing a schema (or said another way, 5% of rules with options are missing a schema). The percentages are only slightly higher (worse) when the top 1,000 ESLint plugins are considered (the top 1,000 results include many repositories that haven't been maintained nor used in years).
  • For the top 100 ESLint plugins, 94% of rules are object rules. The percentage is only slightly lower (worse) when the top 1,000 ESLint plugins are considered.

These findings seem very favorable toward:

  • Requiring rules with options to provide schemas in ESLint 9
  • Removing support for function-based rules in ESLint 9

In order to move forward with both of these breaking changes:

  1. Write an RFC for each
  2. In a minor version of ESLint 8, add deprecation warnings when running tests on a rule that does not meet the new requirements
  3. Utilize eslint-plugin-eslint-plugin to help rule authors prepare as mentioned in the above comment (Change default rule schema to schema: [] and drop support for function-style rules #14709 (comment))

Let me know what you think of this analysis and plan. If it sounds good, I could start on RFCs.

╔══════════════════════════╤═════════════════════════╤═══════════════════════════════════════════════╤════════════════════════════════════════════════╤══════════════════════════╗
║ Metric                   │ Value (Top 100 Plugins) │ Value (Top 1000 Plugins, Updated Last 1 Year) │ Value (Top 1000 Plugins, Updated Last 2 Years) │ Value (Top 1000 Plugins) ║
╟──────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┼────────────────────────────────────────────────┼──────────────────────────╢
║ Plugins Found            │ 100                     │ 639                                           │ 777                                            │ 1000                     ║
╟──────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┼────────────────────────────────────────────────┼──────────────────────────╢
║ Plugins With Rules Found │ 79                      │ 446                                           │ 533                                            │ 677                      ║
╟──────────────────────────┼─────────────────────────┼───────────────────────────────────────────────┼────────────────────────────────────────────────┼──────────────────────────╢
║ Average Rules Per Plugin │ 22.09                   │ 7.96                                          │ 7.23                                           │ 6.27                     ║
╚══════════════════════════╧═════════════════════════╧═══════════════════════════════════════════════╧════════════════════════════════════════════════╧══════════════════════════╝

╔══════════════════════════════════════════════════════════════════╤═════════════════════╤═══════════════════════════════════════════╤════════════════════════════════════════════╤══════════════════════╗
║ Metric                                                           │ % (Top 100 Plugins) │ % (Top 1000 Plugins, Updated Last 1 Year) │ % (Top 1000 Plugins, Updated Last 2 Years) │ % (Top 1000 Plugins) ║
╟──────────────────────────────────────────────────────────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Rules With Options                                               │ 27.16               │ 27.84                                     │ 26.77                                      │ 26.46                ║
╟──────────────────────────────────────────────────────────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Rules With Options But Missing Schema, Out of Total Rules        │ 1.49                │ 1.91                                      │ 1.95                                       │ 2.26                 ║
╟──────────────────────────────────────────────────────────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Rules With Options But Missing Schema, Out of Rules With Options │ 5.49                │ 6.88                                      │ 7.27                                       │ 8.55                 ║
╚══════════════════════════════════════════════════════════════════╧═════════════════════╧═══════════════════════════════════════════╧════════════════════════════════════════════╧══════════════════════╝

╔═══════════════╤═════════════════════╤═══════════════════════════════════════════╤════════════════════════════════════════════╤══════════════════════╗
║ Rule Type     │ % (Top 100 Plugins) │ % (Top 1000 Plugins, Updated Last 1 Year) │ % (Top 1000 Plugins, Updated Last 2 Years) │ % (Top 1000 Plugins) ║
╟───────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Object Rule   │ 93.81               │ 91.41                                     │ 89.74                                      │ 86.80                ║
╟───────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Function Rule │ 4.01                │ 6.14                                      │ 7.84                                       │ 10.91                ║
╟───────────────┼─────────────────────┼───────────────────────────────────────────┼────────────────────────────────────────────┼──────────────────────╢
║ Unknown       │ 2.18                │ 2.45                                      │ 2.41                                       │ 2.29                 ║
╚═══════════════╧═════════════════════╧═══════════════════════════════════════════╧════════════════════════════════════════════╧══════════════════════╝

@nzakas
Copy link
Member

nzakas commented Jul 9, 2021

Wow, that is some fantastic analysis, thank you.

I agree, I think the data supports being able to make this change. I’d suggest putting everything into a single RFC as it will be easier to look at the situation holistically.

@btmills
Copy link
Member

btmills commented Jul 9, 2021

That is amazing 👏 I'm also supportive and feel much better knowing that the vast majority of the ecosystem is already prepared.

@bmish
Copy link
Sponsor Member Author

bmish commented Jul 9, 2021

Thanks for the feedback! I'll work on a single RFC in the near future.

@github-actions
Copy link

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 15, 2021
@bmish
Copy link
Sponsor Member Author

bmish commented Oct 16, 2021

I still plan to work on this hopefully in the near future so we can target ESLint v9.

@bmish bmish removed the Stale label Oct 16, 2021
@nzakas
Copy link
Member

nzakas commented Oct 16, 2021

Awesome, adding you as the assignee to make the bot happy.

@bmish
Copy link
Sponsor Member Author

bmish commented Dec 13, 2021

I have opened up an RFC for this:

@nzakas nzakas added this to Planned in Public Roadmap via automation Dec 14, 2021
@nzakas nzakas added this to Need Discussion in v9.0.0 Dec 14, 2021
@mdjermanovic mdjermanovic moved this from Waiting for RFC to RFC Opened in Triage Jan 27, 2022
@mdjermanovic mdjermanovic moved this from Planned to RFC Under Review in Public Roadmap Jan 27, 2022
@bmish bmish moved this from RFC Opened to Pull Request Opened in Triage Dec 15, 2022
@mdjermanovic mdjermanovic changed the title Change default rule schema to schema: [] Change default rule schema to schema: [] and drop support for function-style rules Jul 21, 2023
@mdjermanovic
Copy link
Member

I updated the title with dropping support for function-style rules, which is included in this change per eslint/rfcs#85.

@nzakas
Copy link
Member

nzakas commented Oct 30, 2023

@bmish we are starting v9 development, so this is ready to be implemented now.

@nzakas
Copy link
Member

nzakas commented Nov 16, 2023

Ping @bmish

@mdjermanovic
Copy link
Member

I'm working on this.

@bmish bmish removed their assignment Dec 27, 2023
Public Roadmap automation moved this from RFC Under Review to Complete Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Public Roadmap
  
Complete
Archived in project
Status: Done
Triage
Pull Request Opened
v9.0.0
Need Discussion
Development

Successfully merging a pull request may close this issue.

4 participants