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

Docs: flat config files / ignores function #18118

Closed
1 task
bradzacher opened this issue Feb 14, 2024 · 14 comments
Closed
1 task

Docs: flat config files / ignores function #18118

bradzacher opened this issue Feb 14, 2024 · 14 comments
Labels
documentation Relates to ESLint's documentation

Comments

@bradzacher
Copy link
Contributor

bradzacher commented Feb 14, 2024

Docs page(s)

https://eslint.org/docs/latest/use/configure/configuration-files-new

What documentation issue do you want to solve?

The docs for both files and ignores state:

files - An array of glob patterns indicating the files that the configuration object should apply to. If not specified, the configuration object applies to all files matched by any other configuration object.
ignores - An array of glob patterns indicating the files that the configuration object should not apply to. If not specified, the configuration object applies to all files matched by files.

I.e. the docs state that the expected types are files: Array<string> and ignores: Array<string>

However the flat config types [1] [2] specify that the two fields instead can also take functions.
I.e. the defined type is type Spec = string | ((string) => boolean), files: Array<Spec> and ignores: Array<Spec>.

The function type was added in DefinitelyTyped/DefinitelyTyped#65530, which references the code for @humanwhocodes/config-array [3].

I spent a bit grepping through this codebase but I can't find any tests for the function form.

So I'm not entirely sure what the correct state is here.
Is the function form of the fields an undocumented feature?

What do you think is the correct solution?

Should the types be updated to remove this or should the docs be updated to document this?

Participation

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

Additional comments

This was reported to us in @typescript-eslint by a user (typescript-eslint/typescript-eslint#8467) because our flat config types were defined based on what the docs specify (i.e. we define Array<string>).

@amareshsm
Copy link
Member

amareshsm commented Feb 14, 2024

I guess the docs need to be updated with the example. files and ignores can take functions also.

files - An array of glob patterns indicating the files that the configuration object should apply to. If not specified, the configuration object applies to all files matched by any other configuration object.
ignores - An array of glob patterns indicating the files that the configuration object should not apply to. If not specified, the configuration object applies to all files matched by files.

I am not sure how to reframe the term An array of glob patterns it looks generic. I will send a PR updating docs and tests for the function form.

@nzakas
Copy link
Member

nzakas commented Feb 14, 2024

The function forms of files and ignores were needed for backwards compatibility with eslintrc (specifically, needed for FlatCompat), but we don't want to encourage people to use them because they are a lot less performant than the string versions. For the string versions, we can do all kinds of optimizations to look for duplicates and modify the patterns to work better -- as soon as functions are put in the middle, we lose a lot of flexibility.

So my preference is to leave this as an undocumented feature.

@bradzacher
Copy link
Contributor Author

Question: why not just deprecate them and remove support entirely instead leaving hidden support?

I've personally never seen anyone use the function form -- surely it's a tiny fraction of a fraction of the ecosystem that needs it.

Very low usage paired with the downsides and maintenance burden of an increased surface area -- from my perspective it seems like a great candidate for removal.

@bradzacher
Copy link
Contributor Author

Investigating this more - it looks like the supported form of files is actually
Array<Spec | Array<Spec>>.
I.e. this is a valid value for files: [[() => true], ['']].

So the docs are even further from the truth than I thought!

@nzakas
Copy link
Member

nzakas commented Feb 15, 2024

Investigating this more - it looks like the supported form of files is actually Array<Spec | Array<Spec>>. I.e. this is a valid value for files: [[() => true], ['']].

So the docs are even further from the truth than I thought!

Yes, and there's a reason for that: if it's not documented, then it's not supported. That's always been the way we've handled documenting our APIs. I'm just not a fan of documenting stuff you're not allowed to use unless there's a replacement for it.

@eslint/eslint-team I could use some other opinions on this.

@bradzacher
Copy link
Contributor Author

Should we fix the DefinitelyTyped types for eslint and remove the "unsupported" versions then so that people don't get the wrong idea?

For context the types I had defined in @typescript-eslint just declared string[] but the DT types declared the other form.

This meant that our flat config types could not accept a config defined with the DT types (because logically an array of functions and strings is incompatible with a target expecting an array of only strings).

So I just had to land a change allow the unsupported types to fix a user pain point.

I'd love to revert that though - but I can only do that if we change the DT types.

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Feb 20, 2024

Making sure I understand: is it accurate to say based on #18118 (comment):

  • These are @deprecated, in the sense that users shouldn't be using or relying on them?
  • Once eslintrc / FlatCompat is dropped for v10, there will be no motivation to keep it?

If both of those are right then it seems reasonable to me that they can be marked as @deprecated in the DefinitelyTyped types for v9, then removed in the types for v10.

not a fan of documenting stuff you're not allowed to use unless there's a replacement for it.

I think that makes a lot of sense for regular docs (e.g. eslint.org). But there are some folks who rely on the undocumented, not-allowed Array<Spec | Array<Spec>> behavior - to the point where it's showed up in the DefinitelyTyped types. For working runtime APIs that users can discover, I think the only way to stop folks from feeling confident using them is to explicitly mark them as @deprecated. Then they'll show up as strikethrough in editors and cause lint errors with users of eslint-plugin-deprecation.

@bmish
Copy link
Sponsor Member

bmish commented Feb 20, 2024

Omitting this particular format from the documentation and annotating the type with @deprecated, assuming removing it is the eventual plan, makes sense to me. I think it's okay to be selective about what functionality we advertise, especially with something like this that's only intended as a temporary workaround.

I lean toward keeping the TypeScript type for it in place until the feature is actually removed, so as not to inhibit people from writing whatever TypeScript code they prefer.

@bradzacher
Copy link
Contributor Author

My big question would be "do we have any known usecases of these deprecated forms?"
If we're sitting here purely speculating -- "maybe out there somewhere someone is using that old, long, long, long deprecated form..." that seems... wrong to me? Wrong that it's still supported in v9. That's my 2c though as a 3rd party.

@bradzacher
Copy link
Contributor Author

That aside.
Looking deeper at the types, I see that the old eslintrc config was defined with string | string[] for both files, excludedFiles, and ignorePatterns:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e26919eb3426f5ba85fed394c90c39efb217037a/types/eslint/index.d.ts#L1024-L1034
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e26919eb3426f5ba85fed394c90c39efb217037a/types/eslint/index.d.ts#L1045-L1050

It's only the new flat configs that were declared with this deprecated form.

And looking at the PR that added that type - the only reason that got added was because the person physically read the code from @humanwhocodes/config-array to determine what it supported. (not sure why they didn't just use the docs but 🤷‍♂️).

Given the old config form doesn't include the deprecated types, I propose that we update the flat config types to also not include the deprecated types. Nobody ever complained that the old types didn't include it so seems weird to add it to the new types just so we can mark it as @deprecated

@nzakas
Copy link
Member

nzakas commented Feb 21, 2024

Given the old config form doesn't include the deprecated types, I propose that we update the flat config types to also not include the deprecated types. Nobody ever complained that the old types didn't include it so seems weird to add it to the new types just so we can mark it as @deprecated

I agree. "If it ain't documented, don't assume it exists even if you dig into the code."

Copy link

Oops! It looks like we lost track of this issue. 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 Mar 22, 2024
@JoshuaKGoldberg
Copy link
Contributor

FWIW I'm in agreement with removing them. If they were never intended for public use, then that's a bug in @types/eslint!

@github-actions github-actions bot removed the Stale label Mar 23, 2024
@nzakas
Copy link
Member

nzakas commented Mar 25, 2024

There's general agreement that this should be considered a bug in @types/eslint, so closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Relates to ESLint's documentation
Projects
Archived in project
Development

No branches or pull requests

5 participants