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

bug(import/namespace): (failing test available) namespace + variable declaration merge "not found in imported namespace" #2861

Open
llllvvuu opened this issue Aug 19, 2023 · 0 comments

Comments

@llllvvuu
Copy link

llllvvuu commented Aug 19, 2023

Here is a popular file that breaks (16 million weekly downloads): https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/body-parser/index.d.ts

Failing test case: #2860

The docs say:

Currently, this rule does not check for possible
redefinition of the namespace in an intermediate scope. Adherence to the ESLint
`no-shadow` rule for namespaces will prevent this from being a problem.

no-shadow doesn't apply here (it shouldn't - this file successfully compiles and declaration merges under tsc), since it's a declaration merge and not a shadow. In the case of @types/body-parser, a variable + namespace declaration merge is necessary (DefinitelyTyped/DefinitelyTyped#57168), since deprecating function overloads is weird in TypeScript: microsoft/TypeScript#40007.

Also, even if it were a shadow, it's a bit harder to adhere when it's in a third-party package.

I could look into adding merging to:

ExportMap.for = function (context) {

Actually merging and redeclaration should have the same implementation in this case, since tsc only allows merging when the namespace has no fields.

Another option could be to deprecate "not found in imported namespace" message since it's a slightly complex feature and I think TypeScript handles it better.

llllvvuu added a commit to llllvvuu/DefinitelyTyped that referenced this issue Aug 19, 2023
Shadowing causes issues with some parsers like `eslint-plugin-import`:
import-js/eslint-plugin-import#2861

Even if those plugins are at fault, this is an improvement in style:

Actually, the namespace is not necessary at all anymore after
DefinitelyTyped#57168

Hover doc looks slightly nicer:
`const bodyParser: BodyParser` instead of `const bodyParser: bodyparser.BodyParser`
llllvvuu added a commit to llllvvuu/DefinitelyTyped that referenced this issue Aug 19, 2023
Shadowing causes issues with some parsers like `eslint-plugin-import`:
import-js/eslint-plugin-import#2861

Even if those plugins are at fault, this is an improvement in style:

Actually, the namespace is not necessary at all anymore after
DefinitelyTyped#57168

Hover doc looks slightly nicer:
`const bodyParser: BodyParser` instead of `const bodyParser: bodyparser.BodyParser`

`unnecessary-bind` linter message on `npm test body-parser` is from `master`.
@llllvvuu llllvvuu changed the title bug(import/namespace): (failing test available) doesn't follow shadowed namespace in .d.ts bug(import/namespace): (failing test available) doesn't handle overloaded namespace in .d.ts Aug 19, 2023
@llllvvuu llllvvuu changed the title bug(import/namespace): (failing test available) doesn't handle overloaded namespace in .d.ts bug(import/namespace): (failing test available) doesn't handle namespace | interface overload in .d.ts Aug 19, 2023
@llllvvuu llllvvuu changed the title bug(import/namespace): (failing test available) doesn't handle namespace | interface overload in .d.ts bug(import/namespace): (failing test available) doesn't declaration merge Aug 19, 2023
@llllvvuu llllvvuu changed the title bug(import/namespace): (failing test available) doesn't declaration merge bug(import/namespace): (failing test available) namespace + variable declaration merge is ignored Aug 19, 2023
@llllvvuu llllvvuu changed the title bug(import/namespace): (failing test available) namespace + variable declaration merge is ignored bug(import/namespace): (failing test available) namespace + variable declaration merge "not found in imported namespace" Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant