-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
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
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
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 Aug 19, 2023
namespace | interface
overload in .d.ts
llllvvuu
changed the title
bug(import/namespace): (failing test available) doesn't handle
bug(import/namespace): (failing test available) doesn't declaration merge
Aug 19, 2023
namespace | interface
overload in .d.ts
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
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
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:
eslint-plugin-import/docs/rules/namespace.md
Lines 26 to 28 in cd95728
no-shadow
doesn't apply here (it shouldn't - this file successfully compiles and declaration merges undertsc
), 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:
eslint-plugin-import/src/ExportMap.js
Line 306 in cd95728
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.
The text was updated successfully, but these errors were encountered: