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 Request: Add feature flag capabilities to ESLint #18458

Open
1 task done
nzakas opened this issue May 15, 2024 · 13 comments · May be fixed by #18516
Open
1 task done

Change Request: Add feature flag capabilities to ESLint #18458

nzakas opened this issue May 15, 2024 · 13 comments · May be fixed by #18516
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@nzakas
Copy link
Member

nzakas commented May 15, 2024

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

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

Additional comments

No response

@nzakas nzakas added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features labels May 15, 2024
nzakas added a commit that referenced this issue May 17, 2024
@nzakas nzakas self-assigned this May 17, 2024
@mdjermanovic
Copy link
Member

Makes sense to me 👍

@fasttime
Copy link
Member

fasttime commented May 21, 2024

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?

@kecrily
Copy link
Member

kecrily commented May 21, 2024

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

@nzakas
Copy link
Member Author

nzakas commented May 21, 2024

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.

@fasttime
Copy link
Member

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.

@nzakas
Copy link
Member Author

nzakas commented May 23, 2024

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.

@fasttime
Copy link
Member

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.

@nzakas
Copy link
Member Author

nzakas commented May 24, 2024

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:

@fasttime
Copy link
Member

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 👍🏻

@nzakas
Copy link
Member Author

nzakas commented May 27, 2024

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

@nzakas nzakas linked a pull request May 27, 2024 that will close this issue
1 task
@mdjermanovic
Copy link
Member

I think RFC isn't needed for this change.

@fasttime
Copy link
Member

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

@nzakas
Copy link
Member Author

nzakas commented May 28, 2024

In that case: #18516

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Status: Implementing
Development

Successfully merging a pull request may close this issue.

4 participants