8000 Enhancement: Allow specifying properties in TypeOrValueSpecifier · Issue #10740 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]

Skip to content

Enhancement: Allow specifying properties in TypeOrValueSpecifier #10740

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

Open
4 tasks done
koooge opened this issue Jan 29, 2025 · 6 comments
Open
4 tasks done

Enhancement: Allow specifying properties in TypeOrValueSpecifier #10740

koooge opened this issue Jan 29, 2025 · 6 comments
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look

Comments

@koooge
Copy link
Contributor
koooge commented Jan 29, 2025

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-floating-promises/#allowforknownsafepromises

Description

Hi there,
@typescript-eslint/no-floating-promises 's allowForKnownSafePromiseReturns #8404 works well for it from node:test. However, it doesn't help for it.only and it.skip. They are handy during debugging so it would be great to have the ability to ignore them as well.

  "allowForKnownSafePromiseReturns": [
    { "from": "package", "name": "it", "package": "node:test" }
  ]

Fail

import { it } from "node:test";

it.only("...", () => { /* ... */ });

it.skip("...", () => { /* ... */ });

Pass

import { it } from "node:test";

// eslint-disable-next-line @typescript-eslint/no-floating-promises
it.only("...", () => { /* ... */ });

// eslint-disable-next-line @typescript-eslint/no-floating-promises
it.skip("...", () => { /* ... */ });

Additional Info

No response

@koooge koooge added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jan 29, 2025
@JoshuaKGoldberg
Copy link
Member
8000

👍 this makes sense to me as a feature request. The difficult point here is that we don't yet have a way in the shared TypeOrValueSpecifier format to specify a property of something imported by a package.

Copying the current interface from https://typescript-eslint.io/packages/type-utils/type-or-value-specifier#packagespecifier:

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

How do we think this could be augmented to allow both it and it.only? A couple of starting ideas...

{ "from": "package", "name": "it", "package": "node:test", "properties": ["skip", "only"] }
{ "from": "package", "name": ["it", { "object": "it", "properties": ["skip", "only"] }], "package": "vitest" }

@JoshuaKGoldberg JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 3, 2025
@kirkwaiblinger
Copy link
Member

How do we think this could be augmented to allow both it and it.only? A couple of starting ideas...

{ "from": "package", "name": "it", "package": "node:test", "properties": ["skip", "only"] }
{ "from": "package", "name": ["it", { "object": "it", "properties": ["skip", "only"] }], "package": "vitest" }

Could we more or less use the actual JS syntax as the specifier syntax?

{ "from": "package", "package": "node:test", "name": "it" }, 
{ "from": "package", "package": "node:test", "name": "it.only" },
{ "from": "package", "package": "node:test", "name": "it.skip" },

PS - how long until we start accepting arbitrary ESquery syntax 🙃

{ "from": "package", "package": "node:test", "selector": "Identifier[name=it]" }, 
{ "from": "package", "package": "node:test", "selector": "MemberExpression[property=only][object.type=Identifier][object.name=it]" },
{ "from": "package", "package": "node:test", "selector": "MemberExpression[property=skip][object.type=Identifier][object.name=it]" },

@JoshuaKGoldberg
Copy link
Member

Could we more or less use the actual JS syntax as the specifier syntax?

How would this extend to multiple properties? Like (describe|it|test) & only|skip?

ESQuery syntax

😱 I never thought of using ESQuery syntax, that's a really interesting idea...

@kirkwaiblinger
Copy link
Member

Could we more or less use the actual JS syntax as the specifier syntax?

How would this extend to multiple properties? Like (describe|it|test) & only|skip?

ESQuery syntax

😱 I never thought of using ESQuery syntax, that's a really interesting idea...

Good point.....

I mean, the other choice, which at first sounds like it wouldn't be crazy, would be regex.

/(describe|it|test)(\.(only|skip|runIf)?/

But this obviously fails quickly for things like

describe
   .it(
       'foo',
        () => {}
   );

I guess the question is would we want to be very general at the expense of being very unwieldy (direction of ESQuery), or very straightforward at the expense of being very constrained (direction of the syntaxes you've described). Trying to hit something in between seems like we'll just do a bad job at a complex problem (like the regex idea).

@JoshuaKGoldberg JoshuaKGoldberg marked this as a duplicate of #11083 May 12, 2025
@JoshuaKGoldberg JoshuaKGoldberg changed the title Enhancement: [no-floating-promises] allowForKnownSafeCalls for test.only Enhancement: Allow specifying properties in TypeOrValueSpecifier May 12, 2025
@JoshuaKGoldberg
Copy link
Member

See also @Harpush's #11083 - another use case for this feature would be specifying specific properties in @typescript-eslint/no-deprecated:

For example in lib dom I want to allow keyCode under KeyboardEvent

Coming back to this after a few months, I think it's fine to take the "straightforward at the expense of being very constrained" approach. Properties are kind of a special case for allow options / TypeOrValueSpecifier because they are only defined in terms of parent type + property name.

My current proposal: let's go with properties?: string[] on the format to switch the target of a TypeOrValueSpecifier from a parent type to specifically those properties?

My only concern here is that feels a little ambiguous. Users might look at it and wonder if properties switches the target or augments it... Does anybody have any ideas on how to resolve this concern?

@Harpush
Copy link
Harpush commented May 17, 2025

How about:

{ "from": "lib", "allowedName": "KeydownEvent" }

and

{ "from": "lib", "name": "KeydownEvent", "allowedProperties": [ "keyCode" ] }

It changes quite a bit the type semantics but it is very straightforward....

Another option is just adding a note about it in docs that properties override the name (switching) and to allow both you need to specify both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look
Projects
None yet
Development

No branches or pull requests

4 participants
0