eslint icon indicating copy to clipboard operation
eslint copied to clipboard

Change Request: Add feature flag capabilities to ESLint

Open nzakas opened this issue 1 year ago • 6 comments

ESLint version

HEAD

What problem do you want to solve?

When rolling out flat config, we ended up using an environment variable to let people opt-in. However, that is fairly limited in that you need to be running the CLI on a command line, so those who were using ESLint elsewhere (via the API or in an editor) need to use different methods of enabling flat config.

As we move forward, it seems like the ability to implement changes and let people opt-in will become increasingly important, and without a standard way to do this, we'll end up implementing special opt-in functionality on a per-feature basis.

What do you think is the correct solution?

Introduce feature flags into ESLint.

For the CLI, we'd add a --flag option that lets people pass these in:

npx eslint --flag v10_feature foo.js

For ESLint class, we'd add a new constructor option:

const eslint = new ESLint({
    flags: ["v10_feature"]
});

For the Linter class, we'd also add a new constructor option:

const linter = new Linter({
    flags: ["v10_feature"]
});

Participation

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

Additional comments

No response

nzakas avatar May 15 '24 15:05 nzakas

Makes sense to me 👍

mdjermanovic avatar May 20 '24 18:05 mdjermanovic

How do you plan to handle opt-in features that require additional configuration? In v8 the environment variable ESLINT_USE_FLAT_CONFIG would take one of two values plus the default, i.e. true = use flat config, false = use eslintrc, or else = detect automatically. So this was not just a feature that one could turn on with a flag, but rather a three-state setting.

If an opt-in feature needs an additional option to indicate how it should work, will the presence of the option be sufficient to enable the feature, or should users specify both a flag and the option?

fasttime avatar May 21 '24 08:05 fasttime

Should it also be allowed to opt-in in eslint.config.js?

kecrily avatar May 21 '24 08:05 kecrily

How do you plan to handle opt-in features that require additional configuration? In v8 the environment variable ESLINT_USE_FLAT_CONFIG would take one of two values plus the default, i.e. true = use flat config, false = use eslintrc, or else = detect automatically. So this was not just a feature that one could turn on with a flag, but rather a three-state setting.

@fasttime The simple answer: we just wouldn't do features like this anymore. We were trying to figure out how to best enable people to use flat config and that's what we came up with. If we had this feature flag functionality, we probably would have just created a flag like v9_config to let people opt-in.

If an opt-in feature needs an additional option to indicate how it should work, will the presence of the option be sufficient to enable the feature, or should users specify both a flag and the option?

Short answer: in order to enable a feature, the flag must be present. Can you give a concrete example of what you're describing?

Should it also be allowed to opt-in in eslint.config.js?

@kecrily No. This is a session-specific setting, like --fix, which cannot be set in eslint.config.js.

nzakas avatar May 21 '24 14:05 nzakas

How do you plan to handle opt-in features that require additional configuration? In v8 the environment variable ESLINT_USE_FLAT_CONFIG would take one of two values plus the default, i.e. true = use flat config, false = use eslintrc, or else = detect automatically. So this was not just a feature that one could turn on with a flag, but rather a three-state setting.

@fasttime The simple answer: we just wouldn't do features like this anymore. We were trying to figure out how to best enable people to use flat config and that's what we came up with. If we had this feature flag functionality, we probably would have just created a flag like v9_config to let people opt-in.

If an opt-in feature needs an additional option to indicate how it should work, will the presence of the option be sufficient to enable the feature, or should users specify both a flag and the option?

Short answer: in order to enable a feature, the flag must be present. Can you give a concrete example of what you're describing?

Thanks for the clarifications. The example I had in mind was exactly the one of the environment variable to control the config type. This is mentioned in the issue description to show the limitations of using an environment variable to opt in to a feature (flat config), but it made me also think about the limitations of not having a value (like "true", "false" or undefined) associated to the flag. I assumed that an additional option would be still necessary to convey that information, alongside the flag.

fasttime avatar May 22 '24 10:05 fasttime

I assumed that an additional option would be still necessary to convey that information, alongside the flag.

I'm not quite sure what you mean. Feature flags are either specified, and so are on, or are not specified, and so are off. Again, I don't think the way we did flat config should be an example of the best way to introduce an experimental feature.

nzakas avatar May 23 '24 13:05 nzakas

I'm not quite sure what you mean. Feature flags are either specified, and so are on, or are not specified, and so are off. Again, I don't think the way we did flat config should be an example of the best way to introduce an experimental feature.

Unfortunately, flat config support is the only experimental feature I have known in ESLint, so I don't have a better example. I am used to other environments where feature flags are regular options and so they can accept values of several types. Node.js has a few of them, so you could run it for example with --experimental-default-type=module. I understand that this outside the scope of what feature flags in ESLint are for.

fasttime avatar May 23 '24 19:05 fasttime

Ah, I see what you mean. To me, the Node.js approach is more difficult for use because we'd need some way to coordinate passing all of that around internally, meaning we'd need to make a bunch of changes each time a new experimental feature was introduced.

With feature flags, we just have one system, so any time we want to add a new feature, we just define a new flag and the relevant code checks for the presence of that flag. We don't need to go through the trouble of adding new parameters to methods, etc.

This feature was inspired by what Remix does:

nzakas avatar May 24 '24 14:05 nzakas

With feature flags, we just have one system, so any time we want to add a new feature, we just define a new flag and the relevant code checks for the presence of that flag. We don't need to go through the trouble of adding new parameters to methods, etc.

Yes, that's indeed an advantage 👍🏻

fasttime avatar May 25 '24 11:05 fasttime

This looks to be a fairly small, non-breaking change. Do we want to do an RFC?

nzakas avatar May 27 '24 15:05 nzakas

I think RFC isn't needed for this change.

mdjermanovic avatar May 27 '24 18:05 mdjermanovic

I'd say a PR is sufficient to discuss this change.

fasttime avatar May 28 '24 05:05 fasttime

In that case: https://github.com/eslint/eslint/pull/18516

nzakas avatar May 28 '24 14:05 nzakas

  1. Is it only used to introduce experimental features, or is it potentially used for others?
  2. unlike others, experimental features, imho, is allowed to introduce breaking changes in non-major versions?
  3. if a feature goes to a stable state, will the corresponding --flag be removed?

aladdin-add avatar Jun 05 '24 02:06 aladdin-add

This can be used for any feature that we want to roll out without making it a full feature in the current release line.

Instead of "experimental", I think a better word is "unstable". So an unstable feature would be one that is still under development and may change.

Once a feature becomes stable, it will either be promoted to not need a flag anymore or to a flag indicating the major release it will be formally released.

So, when we implement https://github.com/eslint/eslint/issues/18385, we should start with a flag of unstable_config_lookup when it's ready for testing but not ready for full release. When the implementation is complete, it would become v10_config_lookup because it can't formally be enabled until v10 as a breaking change.

Then in v10, the flag is removed as no longer necessary.

nzakas avatar Jun 05 '24 14:06 nzakas