10000 RFC: Common format to specify a type or value as a rule option · Issue #5271 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

RFC: Common format to specify a type or value as a rule option #5271

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.

Sign up for GitHub

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

Jump to bottom
Closed
JoshuaKGoldberg opened this issue Jun 28, 2022 · 18 comments
Closed

RFC: Common format to specify a type or value as a rule option #5271

JoshuaKGoldberg opened this issue Jun 28, 2022 · 18 comments

Comments

@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Jun 28, 2022 •
edited
Loading

Overview

There are quite a few times when users have a real need to change or disable a rule for specifically some package exports:

This RFC proposes a unified format that all rules can use to specify some types or values from the global namespace and/or packages.

Prior Art

Some rules have previously created name-based overrides, such as typesToIgnore from no-unnecessary-type-assertion. Those are sub-optimal:

  • Names have conflicts. If you configure a rule for, say, all functions of a particular name that a package exports, you might also happen to ignore the rule for your internal functions of the same name.
  • It may be necessary to configure a rule for all values of a particular type, regardless of their name.

For example, @typescript-eslint/no-floating-promises even has a void operator to get around needing to disable the rule for specific cases. It's the workaround we currently recommend for issues such as #5231.

Proposal

The current proposal is for specifying the following types of sources, in increasing order of distance from a line of code:

  • File: the current file or another specific file in your package (often exported as a module, but not necessarily)
  • Lib: globals such as from lib.d.ts or lib.dom.d.ts
  • Package: an export from a package, such as "lodash"

Additionally, you can specify multiple of these at the same time with type many.

interface FileSpecifier {
  from: "file";
  name: string | string[];
  source?: string;
}

interface LibSpecifier {
  from: "lib";
  name: string | string[];
}

interface PackageSpecifier {
  from: "package";
  name: string | string[];
  source: string;
}

interface ManySpecifiers {
  from: EntitySpecifierFrom[];
  name: string;
}

type EntitySpecifierFrom = "file" | "lib" | "package";

type EntitySpecifier =
  | string
  | FileSpecifier
  | LibSpecifier
  | PackageSpecifier
  | ManySpecifiers;

An example of how this might look in practice:

export default {
  rules: {
    // https://github.com/typescript-eslint/typescript-eslint/issues/5231
    "@typescript-eslint/no-floating-promises": ["error", {
      "ignore": [
        {
          "from": "node:test",
          "name": ["it", "test"]
        }
      ]
    }],
  }
}

See #5271 (comment) and following comments for more context.

Original format, not rich enough

I propose the format look be able to specify whether the item is from the global namespace (i.e. globalThis, window) or a module (e.g. "node:test", "./path/to/file"):

interface GlobalSpecifier {
  name: string;
}

interface ModuleSpecifier {
  export: string;
  module: string;
}

type Specifier = GlobalSpecifier | ModuleSpecifier;

Examples

For example, specifying the global location object:

{
  "name": "location"
}

Instead specifying the global window.location object:

{
  "name": "window.location"
}

Specifying the test export from the node:test module:

{
  "export": "test",
  "module": "node:test"
}

Specify the SafePromise export from some ~/utils/promises module (presumably configured by TSConfig paths to point to a file with a location like ./src/utils/promises.ts):

{
  "export": "SafePromise",
  "module": "~/utils/promises"
}

Specify the SafePromise export from some my-safe-promises (TODO: find some unused or useful name) module:

{
  "export": "SafePromise",
  "module": "safe-promises"
}
@bradzacher
Copy link
Member

I would suggest we make it a proper discriminated union to disambiguate the types and the configuration.

Eg

interface GlobalSpecifier {
  type: 'global';
  name: string;
}

interface ModuleSpecifier {
  type: 'module';
  export: string;
  module: string;
}

type Specifier = GlobalSpecifier | ModuleSpecifier;

@bradzacher
Copy link
Member

The difficult thing for the module spec would be specifying exactly which module you're talking about.

Sure it works fine for npm packages - but how do you handle relative imports? How do you handle tsconfig path mapping?

Perhaps we require the module is either an exact match (eg 'eslint') or an absolute path (eg '/Users/josh/project/src/Foo'). Perhaps we allow for a config option which passes in the project root and it resolves all relative paths relative to that path? Or we could just say "stuff it" and force people to use require.resolve or join path.join(__dirname, './relative)`.

Path mapping is a whole other issue to think about because ideally you would want to specify the absolute path once - and not specify it plus any path mapped versions.

@JoshuaKGoldberg
Copy link
Member Author

proper discriminated union

Hmm, that would make the types easier to work with in code, and explicitly easier to read. But I wonder if it'd get annoying for consumers using it?

relative imports? ... tsconfig path mapping?

I like the "stuff it" option for now. We can always add in relativity later, once we better understand how it's used in user code.

Another thing to consider: users may want to use * wildcards in their paths. I think we should support this. Use cases like explicit-module-boundary-types are well served by it, such as ignoring types on all exports under ./src/pages/.

@bradzacher
Copy link
Member

But I wonder if it'd get annoying for consumers using it?

IMO verbose but clear is better for config. Esp since eslint configs are really written just once.

Use cases like explicit-module-boundary-types are well served by it, such as ignoring types on all exports under ./src/pages/.

I think that sort of usecase would be outside the scope of this RFC wouldn't it?

@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented Aug 1, 2022 •
edited
Loading

I don't think it'd be outside of the scope, no. Additional use case: let's say a project has several variants of Promises -BluebirdPromise, JQuery2Promise, JQuery3Promise, Promise, PromiseLike-. They'll probably want to use a matcher like "*Promise*" on several rules.

I'm also wondering, maybe we should allow plain strings? Like "location" instead of { "name": "location" }?

@bradzacher
Copy link
Member

They'll probably want to use a matcher like "Promise" on several rules.

Personally I'd respond the same way as with the discriminated union - configure once so verbosity is better.

The problem with adding "glob" matchers like that is that if you have positive matchers, you need negative matchers too. Or else people can get stuck trying to craft the perfect string that hits all of your cases but ignores Promise2Cheese.

This is what I found in naming-convention and is why it uses an object to specify regex matches like {regex: Regex, match: boolean}.

I don't think it'd be outside of the scope

Sorry, could you clarify how this RFC would apply to the rule? Are you suggesting we'd make the rule type-aware so that we can support not adding certain return types?

I'm also wondering, maybe we should allow plain strings?

Same as above - one way to configure things with a clear, unambiguous schema is better than allowing shorthand.
We can always add complexity later in a minor if people demand it - but you cant take complexity away without a major.

@JamesHenry
Copy link
Member

I think it's a great suggestion, and I'm also personally in the clear and verbose camp!

PS Where can I install Promise2Cheese from? 😛

@JoshuaKGoldberg
Copy link
Member Author

It just occurred to me that existing rules as mentioned in the OP essentially already use the single string form. So it'd be a breaking change to remove that option.

This was referenced Sep 17, 2022
Closed
Open
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Sep 26, 2022
Closed
4 tasks
@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Oct 26, 2022
Merged
6 tasks
@marekdedic
Copy link
Contributor

Hi,
I'm coming over from the perspective of #4436 and maybe don't have the full context, so sorry if I miss anything important.

Just for posterity, in #4436 we arrived at the following format (reformated to match this RFC):

interface LocalSpecifier {
  source: 'local';
  typeName: string;
}

interface DefaultLibSpecifier {
  source: 'default-lib';
  typeName: string;
}

interface PackageSpecifier {
  source: 'package';
  typeName: string;
  package: string;
}

type Specifier = LocalSpecifier | DefaultLibSpecifier | PackageSpecifier;

IMHO, discriminated unions are the better option, for the reasons outlined in previous comments.

I think the distinction of global vs. module is unfortunately a poor one for several reasons:

  1. The ability to distinguish between a local type (e.g. from the same file/module) and a type from the default lib is lost, if I understand it correctly as both MyAwesomeType and HTMLElement would use GlobalSpecifier, right? I think it would be better to split those two (I am not sure whether Global would then be basically Local?)
  2. Not all code uses modules. While this is gradually changing, I think support for non-module code shouldn't be dropped just yet. In feat(eslint-plugin): [prefer-readonly-parameter-types] added an optional type allowlist #4436 we instead have package vs. local, but that seems too coarse if you use modules... Maybe something like this?
  type Specifier = LocalSpecifier | ModuleSpecifier | DefaultLibSpecifier | PackageSpecifier;

Where if you don't use modules, all of your project code would be Local? Maybe this is too complicated and would just confuse everybody? I am not sure about the solution, but the point about supporting non-module codebases stands.

Additionally, it would be really nice if the same convention as in this RFC (whatever it ends up being) were to be adopted by eslint-plugin-functional e.g. in their rule prefer-immutable-types - CCing @RebeccaStevens as she seems to be behind that rule...

@JoshuaKGoldberg
Copy link
Member Author

@marekdedic yes! Agreed - looking back on my RFC as written, I agree it's too ambiguous between the different sources of specifiers. Just confirming I understand what you're proposing, there are the following types of sources, in increasing order of distance from a line of code:

  • Local: the current file
  • Module: another specific file in your package (often exported as a module, but not necessarily)
  • Package: an export from a package, such as "lodash"
  • Default lib: globals such as from lib.d.ts or lib.dom.d.ts

Assuming we're on the same page, I like the discriminated type union as you've put it, with the addition of ModuleSpecifier. A few thoughts:

  • For LocalSpecifier and ModuleSpecifier, we would want to be able to specify both the name being targeted and a file/module inclusion list. Maybe, source for the actual source (file path / module name), and specifier as the discriminant?
    • Maybe sources should all be optional and default to "*"? I.e. you can ban all, say, local types/values by a name, or in specific file(s)?
    • I don't see a reason why one would want to include source for default lib, though it could have an impact - e.g. you want to ban a specific type from just lib.dom.d.ts...? I'll leave that out for now.
    • I like package as the type name for PackageSpecifier, but it's nice to have the same name for all equivalent properties if possible IMO.
  • I'll nit that typeName isn't ideal because these should be usable for values as well - maybe entity would work?
  • My instinct seeing "default lib" would be that we can simplify to "lib", since it targets lib*.d.ts files... but I can also see an argument that "lib" as a term also often refers to other node modules...
    • Context I'm thinking of primarily: TypeScript used to call its "skip checking the libs code" option skipDefaultLibCheck, and now calls it skipLibCheck to also refer to other node modules 😕
interface LocalSpecifier {
  entity: string;
  source?: string;
  specifier: "local";
}

interface ModuleSpecifier {
  entity: string;
  source?: string;
  specifier: "module";
}

interface DefaultLibSpecifier {
  entity: string;
  specifier: "default-lib";
}

interface PackageSpecifier {
  entity: string;
  source: string;
  specifier: "package";
}

type EntitySpecifier =
  | LocalSpecifier
  | ModuleSpecifier
  | DefaultLibSpecifier
  | PackageSpecifier;

type EntitySpecifierType = EntitySpecifier["type"];

Another complication: what if you wanted to target multiple types of specifiers -- say, all *Promise-named types? You'd have to include up to four unique objects with near-duplicate entity/source. It'd be nice to allow formats like:

// Specifies three, just not from packages
{
    "entity": "*Promise",
    "specifier": ["local", "module", "default-lib"]
}

// Specifies all four
{
    "entity": "*Promise",
    "specifier": ["local", "module", "default-lib", "package"]
}

...and then we also have the backwards compatibility + ergonomics issue of a lot of rules trying that kind of "select everything everywhere all at once" with just a string:

// Shorthand for the "all four" case above
"*Promise";

...so I wonder if we should add in these types:

// (add in types from above)

interface ManySpecifiers {
  entity: string;
  specifier: EntitySpecifierType[];
}

type EntitySpecifier =
  | string
  | LocalSpecifier
  | ModuleSpecifier
  | DefaultLibSpecifier
  | PackageSpecifier
  | ManySpecifiers;

What do you think?

It would be great to have this in for v6, which we just started working on this week. 🚀

@marekdedic
Copy link
Contributor

Hi,
I think you got the general idea right and I am glad you liked it :)

Some comments on the details:

I see that you added filename even to LocalSpecifier - Do I understand it correctly that {specifier: "local", entity: "asdf", source: "myfile.ts"} would match any local variable named asdf only inside the file myfile.ts? As opposed to {specifier: "local", entity: "asdf"} matching any variable named asdf in any file, as long as it was defined in the same file where it was used? I am wondering if these are even useful and wouldn't be better served by an ignore comment? I am not sure.

I'm sorry, but I really don't like the idea of the wildcard shorthands. Like, in general. :( I only have the point of view of prefer-readonly-parameter-types, but I would say if you want to whitelist a type from any source, you're holding it wrong.

As for naming, I used the names form my original PR, so definitely not married to them. My 2¢:

  • If ModuleSpecifier is intended for any files, not just modules, why not call it FileSpecifier (and have correspondingly specifier: "file")?
  • typeName isn't great and outright bad if it's supposed to be used for things other than types. I'm not a fan of entity either, though. How about something like name or item?
  • Let's use lib instead of default-lib, agreed.
  • I am against unifying package name and filename under source - package and file are pretty obvious, source isn't. Understandable > simple
  • I don't like specifier either :D How about location?

As far as v6 goes, I'm looking forward to it and kind of tacitly assumed that this big config redesign was supposed to be the breaking change in v6 :)

@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented Oct 29, 2022 •
edited
Loading

added filename even to LocalSpecifier ... wondering if these are even useful

Yeah, I think it's not useful for most rules, but in some it would be very useful. For example, you might want to target...

  • get(ServerSide|Static)Props functions in src/pages/**/* (Next.js docs)
  • jest.mock calls in *.test.*

if you want to whitelist a type from any source, you're holding it wrong

Agreed 😄. I can't think of any modern codebase reason to want this. The closest I can think of is the idea of targeting all *Promise types...

But, for the sake of the other use cases for this standard format (such as the examples above), I think we'll need to throw it in to the format.

If ModuleSpecifier is intended for any files, not just modules, why not call it FileSpecifier (and have correspondingly specifier: "file")?

You're totally right and my proposal is technically inaccurate - module is a subset of file!

...and now I'm wondering, what use case is there for having a delineation between LocalSpecifier and FileSpecifier/ModuleSpecifier? In retrospect, maybe we should merge those to just FileSpecifier? Going back down to just three variants as you proposed. Updated. 😄

How about something like name or item?

Agreed. I think name is the more standard property folks would expect. Updated.

unifying package name and filename under source ... Understandable > simple

Agreed in theory, but - I think it would be useful to support the ManySpecifiers format. If we have a property in ManySpecifiers used for the equivalent of source/file/etc., it feels odd to me that it would be different the individual formats. Maybe I'm just being overly cautious. Do you have thoughts on how we could make property name ergonomic?

I don't like specifier either :D How about location?

Heh, my hesitation there is that loc/location in linting land generally refers to location within a file in rule complaints. Maybe ... from?

On that train of thought, words like in and where might be good to use...

this big config redesign was supposed to be the breaking change in v6 :)

Haha, this is definitely top of mind & high excitement from a technical perspective! I think you're right for power users like you and me (my favorite kind of user ❤️). But for marketing purposes I'm thinking #5204 and #5900 will likely end up being emphasized more. 🥲

@marekdedic
Copy link
Contributor

As for the usefulness of LocalSpecifier, I probably don't have enough context, so 🤷

Agreed smile. I can't think of any modern codebase reason to want this. The closest I can think of is the idea of targeting all *Promise types...

But, for the sake of the other use cases for this standard format (such as the examples above), I think we'll need to throw it in to the format.

If it's supposed to be for edge-cases and outdated code, I would say it's fine to have the user specify it manually. But if you want to do shorthands anyway, I'm much more in favour of the "the specifier can be an array" approach than the "replace the whole object with just a string" approach - the second one is too opaque and I think it would encourage users to do that even if they don't need to, because it looks like the basic format whereas IMO, it should be an edge case use.

...and now I'm wondering, what use case is there for having a delineation between LocalSpecifier and FileSpecifier/ModuleSpecifier? In retrospect, maybe we should merge those to just FileSpecifier? Going back down to just three variants as you proposed. Updated. smile

Ha, I thought about that as well since writing my last comment, but I arrived at the opposite conclusion - there is a meaningful difference. See this (extremely simplified) example of a non-module file:

function showLessonEditView() {
    const lessonID = $("lesson").data("id");
    showLessonName(lessonID);
    // ... rest of function
}

function showLessonName(string lessonID) {
    $("body").innerHTML = "This is a lesson with the ID" + lessonID;
}

Even thought this is not a module, there are some variables and functions (and possibly types) that are used just in this file, like lessonID or showLessonName(). For these, it would make sense to only be whitelisted in this file (as lessonID might be a common local variable name), so a LocalSpecifier would be great for this (or an ignore comment...). On the other hand, the function showLessonEditView is probably supposed to be used from outside this file, so it would make sense to use FileSpecifier for it. If this file were a module, only showLessonEditView would be exported and there the same logic could apply - use FileSpecifier for entities that cross module boundaries (i.e. are exported) and LocalSpecifier for ones that don't.

unifying package name and filename under source ... Understandable > simple

Agreed in theory, but - I think it would be useful to support the ManySpecifiers format. If we have a property in ManySpecifiers used for the equivalent of source/file/etc., it feels odd to me that it would be different the individual formats. Maybe I'm just being overly cautious. Do you have thoughts on how we could make property name ergonomic?

Hmm, you're right, it does make sense to have a single name in that format. I'm still against that format overall, but if it's that way, let's keep just one name.

I don't like specifier either :D How about location?

Heh, my hesitation there is that loc/location in linting land generally refers to location within a file in rule complaints. Maybe ... from?

On that train of thought, words like in and where might be good to use...

I think from is great. {name: "HTMLElement", from: "lib"} seems very understandable to me.

@JoshuaKGoldberg JoshuaKGoldberg changed the title RFC: Common method for specifying a type or value as a rule option RFC: Common format to specify a type or value as a rule option Nov 11, 2022
@JoshuaKGoldberg
Copy link
Member Author
JoshuaKGoldberg commented Nov 11, 2022 •
edited
Loading

For these, it would make sense to only be whitelisted in this file ...
On the other hand, the function showLessonEditView is probably supposed to be used from outside this file, so it would make sense to use FileSpecifier for it

What worries me is that it often won't be clear which one to use. Suppose you want to ban showLessonName from the example. You could use either a FileSpecifier or ModuleSpecifier [edit: LocalSpecifier] right? How would we delineate between the two?

(sorry for the delay - still very interested in this conversation!)

@JoshuaKGoldberg
Copy link
Member Author

Also mentioned in ESLint's discussions: eslint/eslint#16540

@marekdedic
Copy link
Contributor

What worries me is that it often won't be clear which one to use. Suppose you want to ban showLessonName from the example. You could use either a FileSpecifier or ModuleSpecifier right? How would we delineate between the two?

Hi, either you meant FileSpecifier and LocalSpecifier or we're talking past each other :) Assuming the first, the difference would be that LocalSpecifier allows you to ban a specifier in the same file and only in that same file. OTOH, FileSpecifier would allow you to ban a specifier when used from other files/modules.

@JoshuaKGoldberg
Copy link
Member Author

Whoops, sorry - edited now 😅 to say LocalSpecifier, thanks for the catch. Given that I'm already mistaking them for each other (which was not planned - a true mistake/typo) and they have so much overlap, I'm going to continue the suggestion that we unify to one. If there's ever a need for a rule we can always add a property like imported?: 'never' | 'only'.

@marekdedic
Copy link
Contributor
74F1

They are very similar, true. I'd lean towards having both, but it's a trade-off that depends on what you prioritize...

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue Nov 16, 2022
Closed
@typescript-eslint typescript-eslint locked and limited conversation to collaborators Nov 17, 2022
@JoshuaKGoldberg JoshuaKGoldberg converted this issue into discussion #6017 Nov 17, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@JamesHenry @marekdedic @JoshuaKGoldberg @bradzacher
0