Change Request: Add API for integrators to easily swap between flat config and eslintrc APIs

[nzakas]

ESLint version

v8, v9

What problem do you want to solve?

Right now, we use the /use-at-your-own-risk entrypoint for accessing non-default objects like FlatESLint and LegacyESLint, but doing so requires integrators to pull from different locations depending on which mode they want to run ESLint in. This is error-prone, especially if they don't know the version of ESLint ahead of time.

What do you think is the correct solution?

Add an API, exported from the main entrypoint, that allows access to the correct APIs and provide that in v8, v9, and v10 to ease integration difficulty.

Potentially move shouldUseFlatConfig to the main entrypoint as an export.

Participation

• I am willing to submit a pull request for this change.

Additional comments

No response

[nzakas]

To summarize what @dbaeumer said:

Reading the link from above brought me to the following conclusion:

* 8.x : flat config is not the default an needs to be enabled via an env variable `ESLINT_USE_FLAT_CONFIG` or when a flat config file exists in the workspace root. Since a lot of VS Code users start VS Code from a dock it is not easy to inject env variables based on workspaces. This is why I introduced the `eslint.experimental.useFlatConfig` since it gives control over this per workspace. We can debate the name (e.g. the experimental part) but I think it is not worse it to change right now. What I can add is code that checks if `ESLINT_USE_FLAT_CONFIG` is set and then automatically set `eslint.experimental.useFlatConfig`.

* 9.x: flat config is the default unless explicitly disabled. Her the fun begins :-). I can only determine the version of ESLint after I have loaded ESLint. If I get < 9.x I need to reload ESlint from `/use-at-your-own-risk` in case users opted into flat config.

* 10.x: more version checks and more if then else code blocks since now the behavior of the classes have changed (ESLint -> FlatConfig).

From my experiences designing API, it would have been easier for API users, if the API would have added new names and removed old names instead if reusing names and changing the meaning. The first would have avoided checking the version since the pure existence of a name would introduce a behavior. In VS Code we usually use a function to load the API to which you can pass a parameter to determine if you want proposed API. If you opt into this they are put into the same namespace.

And also:

Actually I don't want to change the timing. I understand that this is not possible. I was more hoping for some sort of additional API to make this easier for API users that need to support 8.x, 9.x and 10.x

Let me first explain what we do in VS Code and LSP to mitigate those changes. In general we do two things:

* proposed API has NO special name space. This avoid that when we move things to final names stay the same

* we don't reuse `deprecated` names

VS Code

* we patch the node loader. So if you require('vscocde') we consult the extension's package.json file and inject those proposed APIs into the returned literal the extension opted into.

* in LSP the API exists on the client / server. So when you construct these you can pass in flags to tell which of the proposed API you want to use. A good example is here that opts into all proposed new API: https://github.com/microsoft/vscode-languageserver-node/blob/2d33852e06ff9f180abd528be464f02b9bbd3079/testbed/server/src/server.ts#L27

ESLint

For ESLint it would be cool to have the following:

* they top level API (when requiring `eslint`) exports a function `resolveAPI(proposed: boolean)`.

* the return literal would have the following entries

{
     shouldUseFlatConfig?, // Function as described
     ESLint?, // The current ESLint class
     ESLint2?, // The flat eslint class
     Linter?, // The current linter class
     Linter2?: // The flat linter class
}

If proposed is false then the 2 classes and the shouldUseFlatConfig would not be there.

Something like this would allow to use the API without checking version to interpret names differently.

Let me know if something like this would be possible?

[nzakas]

Hmm, I see. I think something like that is doable, but I'm not sure this design makes sense.

First, we only have one Linter class. In v8, that uses eslintrc by default but can be changed with an option; in v9 it uses flat config by default but can be changed with an option; in v10 it uses flat config and the option is ignored because eslintrc is removed. So you can just use Linter as it is returned from the ESLint package today.

Second, the fact that there is a FlatESLint vs. LegacyESLint is an implementation detail. We could have just provided an option to ESLint like we did with Linter, but the code was sufficiently different that we were basically rewriting everything from scratch that it was easier to do a completely separate class so when the time came we could just delete the old one rather than sifting through if statements everywhere.

So, it seems like what you really need is something that allows you to write code like this:

import { loadESLint } from "eslint";

// retrieve FlatESLint
const ESLint = await loadESLint(true);

// or

// retrieve LegacyESLint
const ESLint = await loadESLint(false);

And maybe shouldUseFlatConfig() should just move to be exported from the package itself.

Would that make sense?

[aladdin-add]

I was thinking if we could support an option configType: "flat" | "eslintrc" in ESLint.

// eslint v8
const eslint = new ESLint();  // defaults to configType: "eslintrc"

// eslint v9
const eslint = new ESLint();  // defaults to configType: "flat", but "eslintrc" is allowed

// eslint v10
const eslint = new ESLint();  // defaults to configType: "flat", and "eslintrc" is disallowed.

Users can combine shouldUseFlatConfig and ESLint:

// works for all eslint >=8
const {shouldUseFlatConfig, ESLint} = require("eslint");
const configType = shouldUseFlatConfig() ? "flat" : "eslintrc";
const eslint = new ESLint({configType});

[aladdin-add]

shouldUseFlatConfig is now exported in "eslint/use-at-your-own-risks", but, imho, it's fine to export it in the main entry. as in eslint>= v10, it's just a () => true and doesn't incur any more maintenance costs.

[nzakas]

@aladdin-add We opted not to introduce a configType option to the ESLint constructor for a few reasons. In this case, we'd actually end up returning an instance of a different class, so eslint instanceof ESLint wouldn't work. The easiest option is just to have a separate function that returns the correct class.

[aladdin-add]

From an ease-of-use perspective, maybe we could call shouldUseFlatConfig() in loadESLint(), which appears to be the case for users:

const { loadESLint } = require("eslint");

const ESLint = await loadESLint();  // FlatESLint or LegacyESLint
const eslint = new ESLint();

[nzakas]

I was thinking something like this:

const { loadESLint } = require("eslint");

const ESLint = await loadESLint({
    useFlatConfig: true, // or false, or undefined to call shouldUseFlatConfig()
    cwd: "."
});

@dbaeumer what do you think?

[dbaeumer]

@nzakas first thanks a lot for looking into this.

I like the proposed approach. But to clarify: if in the future ESLint would add another feature we would add another flag. For example if it would use a new validation engine we would have a flah useNewEngine

[nzakas]

@dbaeumer yes, my proposed design was made with the intent that we could always add more flags if necessary. 👍

[nzakas]

The TSC met today to discuss some further details on this:

• We will also add a configType static property to the ESLint and LegacyESLint constructors to indicate the config type it uses.
• We will need to backport the changes to the v8.x branch so loadESLint() is also available there. That will end up being a slightly different implementation.

@aladdin-add I know you started a PR, but I'm going to create a new one based on the discussions we had today.

[nzakas]

Closing as the PRs for both branches have been merged.