Skip to content

Feature: [readonly-parameter-types et. al.] enhance allow option to allow @types/ built-ins #8572

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
arkapratimc opened this issue Feb 28, 2024 · 7 comments
Open
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@arkapratimc
Copy link
Contributor

arkapratimc commented Feb 28, 2024

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

Relevant Package

type-utils

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).

Description

A few lines of context -

  1. An old rfc - RFC: Common format to specify a type or value as a rule option #6017 (comment)
  2. StyleShit's comment on a PR on which I'm working - feat(eslint-plugin): [no-floating-promises] add an 'allowForKnownSafePromises' option #8502 (comment)

Steps to reproduce -

  1. go to https://stackblitz.com/edit/stackblitz-starters-tnrbtd?file=src%2Findex.ts
  2. run npx eslint src/index.ts in terminal
  3. check index.ts, line 4

Is typescript-eslint open to include a config option like {from: "package", name: "Foo", package: "data:text/javascript, export class Foo {};"}, or with the other URL schemes(like file:), in the eslint config file ?

There are 3 URL schemes in total - node:, file: & data:.
I think node: will be required to solve #8404. I just want to know about the other 2.

Additional Info

No response

@arkapratimc arkapratimc added enhancement New feature or request triage Waiting for team members to take a look labels Feb 28, 2024
@arkapratimc arkapratimc changed the title Enhancement: support Node.js builtin modules in readonly-parameter-types Enhancement: support Node.js builtin modules in prefer-readonly-parameter-types Feb 28, 2024
@bradzacher
Copy link
Member

Is typescript-eslint open to include a config option like {from: "package", name: "Foo", package: "data:text/javascript, export class Foo {};"}, or with the other specifiers(like file:), in the eslint config file ?

Definitely not, no.
First of all there's already the ability to select specific files via the file type. So why would we allow file:// specifiers in the package type?

Please see the rule docs.
https://typescript-eslint.io/rules/prefer-readonly-parameter-types/#options

As for data:text/JavaScript - what does that even mean? That you would just execute random JS? It's honestly non-sensical when the option is looking for an npm package name.

@arkapratimc
Copy link
Contributor Author

arkapratimc commented Feb 28, 2024

Thanks for the quick answer, so its only node: which will be supported in the package option ?

@bradzacher bradzacher added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin accepting prs Go ahead, send a pull request that resolves this issue and removed enhancement New feature or request triage Waiting for team members to take a look labels Feb 28, 2024
@bradzacher
Copy link
Member

bradzacher commented Feb 28, 2024

This isn't an enhancement - it's a bug.

We don't handle the node special scoped packages in this function
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages%2Ftype-utils%2Fsrc%2FTypeOrValueSpecifier.ts#L155-L172

Which means we're looking for @types/node:events.
I'd guess a lot of the node internal packages might fail that lookup given that events maps to @types/node?

@bradzacher bradzacher changed the title Enhancement: support Node.js builtin modules in prefer-readonly-parameter-types Bug: [readonly-parameter-types] allow option does not support Node.js builtin modules Feb 28, 2024
@arkapratimc
Copy link
Contributor Author

We don't handle the node special scoped packages in this function

Yes, I also tried to solve it here in a rough way.

So, the only URL scheme which is allowed, is node: right ?

@auvred
Copy link
Member

auvred commented Feb 28, 2024

What about bun: prefixed packages? https://bun.sh/docs/runtime/bun-apis

@bradzacher
Copy link
Member

Whilst technically they are URL schemes I don't think we should be thinking about then like that.

Instead we should just be treating them as a special namespace case. Eg node:foo maps to @types/node because we see the colon, split it, and take the first part as the namespace to use.

@Josh-Cena
Copy link
Member

Josh-Cena commented Jun 1, 2024

As for data:text/JavaScript - what does that even mean? That you would just execute random JS? It's honestly non-sensical when the option is looking for an npm package name.

Off-topic but here's a trivia:

import { Foo } from "data:text/javascript, export class Foo {};"

console.log(new Foo());

This is valid code :)

@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: [readonly-parameter-types] allow option does not support Node.js builtin modules Feature: [readonly-parameter-types et. al.] enhance allow option to allow @types/ built-ins Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

4 participants