Breaking: Strict package exports

[nzakas]

Summary

Node.js 12 introduced the ability to strictly define package exports that ensure internal package modules cannot be accessed externally. This RFC proposes implementing package exports for the eslint package to prevent unexpected use of internal modules.

Related Issues

eslint/eslint#13654

[uniqueiniquity]

Will deprecated parts of the documented API still be accessible? e.g. CLIEngine.

EDIT: Oops, I should have actually read the change. Sorry for the noise.

[bradzacher]

I think you'd find that pretty much every single external usecase probably hasn't migrated because there's not much drive for people to do so.

One of the largest/most widely used usecases would be vscode-eslint:
https://github.com/microsoft/vscode-eslint/blob/e4b2738e713b7523824e0c72166f5cdd44f47052/server/src/eslintServer.ts#L1445

[nzakas]

Right, which is why we need to remove CLIEngine -- to drive people to upgrade.

I explicitly mentioned vscode-eslint elsewhere in this RFC.

[bradzacher]

Most changes made to rules are entirely compatible with extension rules and they work without any modifications to the extension. So by purely extending a rule instead of forking we can get all of these changes and fixes for free with zero maintenance cost.

The issue with completely forking the rule is that it is a very heavy, labor intensive process - it would require us to explicitly monitor the upstream codebase and manually merge each and every change made to the rules.

Auto merges would likely be impossible due to having to understand how the fork has changed the rule's code. For example in typescript-eslint we add types to code when we fork it which can change the code in ways that machines cannot understand.

This would lead to forks falling behind upstream - which means that users would encounter bugs that are fixed in one version but not in another, or features that are missing. Which would just lead to user frustration.

---

Whilst I don't have a problem with blocking rules from being deep required (require("eslint/lib/rules/eqeqeq")) as that sort of exposure is a deep and explicit exposure of ESLint's internal folder structure, etc.

I don't think they should be completely blocked from being required from the package, and that they should be exposed as a "package" from the package (eg something like const {coreRules} = require('eslint');).

Rules are already considered part of ESLint's public API (ofc, as users consume them via configs), and rule removals are already a breaking change as per eslint's current semver policy. So it shouldn't be an additional maintenance burden to expose rules in this manner.

[ljharb]

I agree with every one of these points.

[aladdin-add]

maybe we can go further: move the core rules to another package (e.g. @eslint/core-rules), just like the eslint --init(rfcs#79)

[mysticatea]

A workaround to get core rules is new Linter().getRules(), but it's relatively slow because it forces to load all core rules then shallow-copy them. (Normally, eslint loads only enabled (== error or warn) core rules lazily.)

[nzakas]

Rules are already considered part of ESLint's public API...

This is just untrue. People have been using rules as if they were part of the public API for a while, and we've always discouraged people from doing this but we had no way to enforce it until now. We simply can't support rules the same way we support documented APIs.

Rules are already considered part of ESLint's public API (ofc, as users consume them via configs), and rule removals are already a breaking change as per eslint's current semver policy. So it shouldn't be an additional maintenance burden to expose rules in this manner.

This is an assumption you're making and not a fact. Yes, removing a rule is a breaking change, but that's not the only type of change we can make. We can require additional meta data or remove existing meta data, we can change the function signature, we can change the overall format (as we already did, from function to object), we can change the messages being produced, we can change the directory structure. The rules are an unstable interface and only designed to be used inside of ESLint.

Each rule we maintain has a maintenance burden, and if we add compatibility with projects who are using rules in ways we don't expect, then that is more of a burden than otherwise.

The issue with completely forking the rule is that it is a very heavy, labor intensive process - it would require us to explicitly monitor the upstream codebase and manually merge each and every change made to the rules.

I'm not sure I understand. Wouldn't you just wait until a new ESLint release rather than monitoring every commit?

Also, I'm not clear on how using a copy of the rule, rather than referencing it from ESLint, could lead to incompatibilities. Take for example comma-dangle. This file references the original ESLint rule and then uses that to create the TypeScript-specific version. It seems like you could accomplish this with copies of rules, but just copying them into a specific directory and referencing them the same way (so require("./original-rules/comma-dangle") instead of require("eslint/lib/rules/comma-dangel")).

I understand that perhaps there would be a lag, though again, it seems like it would be a fairly simply copy operation since you are already referencing (without modification) the rules ESLint is shipping with.

[ljharb]

Copying a rule's code protects against a breaking change, but also prevents every other kind of improvement - bug fixes, performance benefits, new features, etc. It doesn't seem like recommending copy-pasting code is something that would make the ecosystem better, and it would potentially increase triage burden on eslint's own maintainers, as the number of possible causes of a bug would magnify.

[JamesHenry]

@nzakas What you are suggesting would couple typescript-eslint to an exact patch version of eslint, which is a massive change from now where the user is in control of the ESLint that is used at runtime

[JamesHenry]

@ljharb articulated it well in the meantime :)

[bradzacher]

It seems like you could accomplish this with copies of rules, but just copying them into a specific directory and referencing them the same way

We could, and we would need to copy all referenced utilities as well -- not a huge deal, but it's extra code for us to clone.
This would mean we could use automation to sync the ESLint code - which alleviates a decent chunk of the maintenance burden!

Though it's worth noting that any automation we setup would be dependent on the internal folder structure of ESLint which, as we've established, is undesirable.

I'm not sure I understand. Wouldn't you just wait until a new ESLint release rather than monitoring every commit?

Sorry - yes, that's what I meant.

We have to monitor the release notes for changes to rules we have "extended".
If one is changed, we have to review all changes made in the release and decide if they need to be applied to our fork (if we forked the code - the answer is OFC always "yes").
If they need to be applied, we then need to pull master, branch, apply the changes manually, push a PR, get it reviewed and merge it.

Which is where the non-trivial maintenance burden lies.

This also assumes that changes to ESLint's internal utilities are correctly labelled as impacting all rules that touch those utilities in the release notes so that we can pick them out correctly. If the team is anything like me - then a the broader impact of a change to a utility function will not really be considered or documented unless a test fails because at the scale of 100s of rules it's hard to manually track and document the impact.

---

To be clear here: I'm not saying that ESLint has to commit to keeping a rule static unless a major release occurs.

All we care about in typescript-eslint is that the rules are exported from the package. Beyond that we can and do already take care of the compatibility issues.
We support multiple major releases of ESLint - meaning that even if you strictly encoded the rules as part of the API - we would still have to do our own work to maintain our compatibility as appropriate.

As an example - as part of ESLint v7 I PR'd ESLint to add a bunch of message ids to rules (eslint/eslint#12715) (and another contributor picked up the rest eslint/eslint#12859). In typescript-eslint because we support both v6 and v7 of ESLint we had to appropriately support this -- and we were happy to do so.

I 100% do not want the declared exports to be the deep folder path. I do not want you to allow require('eslint/lib/rules/comma-dangle'). I do not want anyone to depend on the internal folder structure. What I'm asking for is either const {coreRules} = require('eslint') or (as @aladdin-add suggested) const rules = require('@eslint/core-rules').

If you want to explicitly encode this in your semver policy - please do. Something like this:
"Rules are exported from the ESLint package. The only thing guaranteed between releases is the existence of a rule in the export. We provide no other guarantees. All other parts of the rule - including, but not limited to its format, its messages and its schema are subject to change with or without a major release."

---

Ultimately you all are the maintainers of ESLint - so you have the final decision.

I'm just flagging that there are usecases for extending rules, and there are multiple packages that do this (@typescript-eslint/eslint-plugin, eslint-plugin-vue, eslint-plugin-babel, any of the other 13 consumers of eslint-rule-composer).

If you were to "ban" this usecase altogether - then you would in turn force at least a dozen projects to fork the ESLint codebase. Which could (and very likely will) in turn create weirdness and inconsistencies for several million ESLint users.

[ljharb]

Suggested change

The most import consumer of the `CLIEngine#getRules()` method is the [VS Code ESLint extension](https://github.com/microsoft/vscode-eslint), which is one of the most popular extensions for the editor. Currently, the extension [uses `CLIEngine#getRules()`](https://github.com/microsoft/vscode-eslint/blob/e4b2738e713b7523824e0c72166f5cdd44f47052/server/src/eslintServer.ts#L1395) after running ESLint on a file in order to display additional information about the rule. In this case, it should be easy to switch to the new `ESLint#getRulesForResult()` method to maintain current functionality.

The most important consumer of the `CLIEngine#getRules()` method is the [VS Code ESLint extension](https://github.com/microsoft/vscode-eslint), which is one of the most popular extensions for the editor. Currently, the extension [uses `CLIEngine#getRules()`](https://github.com/microsoft/vscode-eslint/blob/e4b2738e713b7523824e0c72166f5cdd44f47052/server/src/eslintServer.ts#L1395) after running ESLint on a file in order to display additional information about the rule. In this case, it should be easy to switch to the new `ESLint#getRulesForResult()` method to maintain current functionality.

Also, eslint-find-rules relies on this functionality: https://github.com/sarbbottam/eslint-find-rules/blob/c8a71a137e34dd396d63f235230aceef3cf9fe3e/src/lib/rule-finder.js

[ljharb]

Theoretically, you could drop "main" and only use "exports", if you're explicitly requiring a version of node that supports "exports" - however, since there's many tools that don't yet understand "exports", it will likely break a lot of things if you don't also provide a "main".

However, if you do provide a "main", and if you do require a node that supports "exports", dropping the "main" later wouldn't be a breaking change once the tooling ecosystem has caught up.

[nzakas]

Awesome, thanks for the insights.

[ljharb]

https://github.com/sarbbottam/eslint-find-rules/blob/c8a71a137e34dd396d63f235230aceef3cf9fe3e/src/lib/rule-finder.js

[mysticatea]

Looks like it's using Linter#getRules() rather than CLIEngine#getRules().

Linter#getRules() returns only core rules unless Linter#defineRule() method is called.
On the other hand, CLIEngine#getRules() also returns plugin rules that the last CLIEngine#executeOn***() method call used.

[ljharb]

Thanks for clarifying.

it’s probably worth searching GitHub to see how many public projects actually do rely on these internals, rather than relying on people to discover this issue and volunteer use cases.

[nzakas]

Agreed, but there's only so much energy I have available for deep research while also trying to maintain the project. I did some initial research, which is how I ended up with the list of plugins in the RFC. If you know of others, I'd appreciate you commenting on them so I can explore more.

[nzakas]

Thanks for everyone's feedback, after thinking about things some more, and discussing with the TSC, we've decided to make some changes to this proposal.

First and foremost we need to be clear: the TSC does not believe that supporting core rules as part of our public API is a good or desired outcome for all of the reasons I've already mentioned. However, it seems pretty clear that those who are already accessing core rules through undocumented APIs are willing to accept that this is a "void your warranty" situation and they are responsible for monitoring for breaking changes and responding to them without involving the ESLint team.

As a result, this latest version of the RFC introduces a new entrypoint called use-at-your-own-risk, where we will publish APIs that are not supported or guaranteed in any way. As part of that, there will be a rules API that grants access to ESLint's core rules. Because rules is a Map, it removes the dependence on the file system and directory structure to allow access to rules, which means that consumers will no longer have to worry about what happens if we change the directory structure. If we find other undocumented APIs that are critical to remain available, we can also add those use-at-your-own-risk with the same caveats.

Again, to be clear, anything in the use-at-your-own-risk entrypoint is considered unsupported and may contain breaking changes at any point in time. It's up to consumers of these APIs to monitor ESLint releases for possible breakages.

[btmills]

If the package maintainers who consume the ESLint API are happy with this version, then it works for me too.

[dbaeumer]

What is your schedule to release v8.0.0. The biggest problem I have with removing the old CLIEngine is that I as an VS Code extension owner (e.g. the ESLint extension for VS Code) still have to support the old API. Users can still rely on ESLint version < 8.0.0 in their workspace which will not have the new API especially not ESLint#getRulesForReport(report) So I need to write probing code and then do the right thing depending on the ESLint version installed. This will be quite some code effort on my end.

Can the old API still be accessible via the se-at-your-own-risk entry point?

[nzakas]

@dbaeumer can you check to see if CLIEngine is present and if not, use ESLint? Or is there some complexity here that I’m not seeing? (We are exporting a CommonJS module so I’m assuming that such a check is fairly easy, but please let me know if I’m missing something about your use case.)

[nzakas]

Oops, I realized I forgot to answer the first question. The timeline for v8.0.0 is not set, but my guess is most likely around two months from now. This RFC must remain open for at least two more weeks and we also need to finish up the other tasks, including an Espree update.

[dbaeumer]

@nzakas Checking for CLIEngine versus ESLint is not complicated. But to my knowledge the new API is async whereas the old one is sync. So there is more than a simple if :-)

[dbaeumer]

And thanks for the timeline forecast.

[nzakas]

@dbaeumer can you explain more about why the change to async is problematic? I’m envisioning that you could normalize this by creating a CLIEngine wrapper that just wraps the method call return values with Promise.resolve(). I’d be happy to help with such a transition.

[dbaeumer]

It is not problematic. It is simply coding effort. And usually it ripples through the call stack (functions that call async API need to become async as well).

[nzakas]

@dbaeumer Gotcha. Well, as I mentioned, I'm happy to help with that transition to ease the burden on you. I'll be in touch when the first v8.0.0 prerelease is out and we can go from there.

[nzakas]

Moving to final commenting.

[ljharb]

Given that rules must operate synchronously, an api moving to async means that it can’t be used by rules.

[nzakas]

@ljharb That is a true system but i can’t imagine a scenario where a rule would use the ESLint class.

[ljharb]

Please be sure "./package.json": "./package.json" is in "exports", otherwise every tool that looks at eslint's version (eslint-plugin-import, eslint-plugin-react, eslint-plugin-jsx-a11y, etc) will break, as well as most bundlers.

eslint-plugin-import currently uses these APIs in rules:

• require('eslint/lib/cli-engine/file-enumerator').FileEnumerator

eslint-plugin-react currently uses these APIs in tests, using RuleTester.linter.defineRule:

• eslint/lib/rules/no-undef
• eslint/lib/rules/no-unused-vars
• eslint/lib/rules/prefer-const

eslint-find-rules uses these APIs:

• eslint/lib/shared/relative-module-resolver
• require('@eslint/eslintrc').Legacy.naming

(The things here that use rules only need an opaque object that can be passed to defineRule)

[nzakas]

@ljharb thank you. 🙏 I've updated the RFC to reflect these.

The relativeModuleResolver is now available in the @eslint/eslintrc package as ModuleResolver, so that should cover eslint-find-rules.

Is there a reason eslint-plugin-import uses FileEnumerator instead of a general-purpose globber? This will ultimately go away when we move to the new config as we will be able to use something like globby instead.

[ljharb]

@nzakas the reason is because we want to always use the exact identical globbing method eslint is using, which includes skipping eslintignore - there's no guarantee a general-purpose globber will match what eslint does.

Any solution for the defineRule RuleTester case?

[aladdin-add]

RuleTester is a public API, so it is usable. am I missing something?

[nzakas]

@ljharb hmmm okay, we should talk more about that down the line because I'd like to get rid of FileEnumerator and just use something off-the-shelf.

And as @aladdin-add mentioned, RuleTester is already part of the public API and remains so, and eslint-plugin-react can use the use-at-your-own-risk entrypoint to access the rules it's using.

[nzakas]

The final commenting period has ended, so merging. We can iterate more when there is a v8.0.0 prototype to try out and see what issues we run into.

[ljharb]

@aladdin-add RuleTester is fine, it’s that defineRule needs to take a rule object.