Skip to content

import statement from "node" in js source file could produce correct declaration path. #41816

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

Merged
merged 3 commits into from
Dec 7, 2020

Conversation

ShuiRuTian
Copy link
Contributor

Fixes #41800

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Dec 4, 2020
@ShuiRuTian

This comment has been minimized.

export default Foo;
declare class Foo extends EventEmitter {
}
import { EventEmitter } from "events";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so the key point to this test is that this .d.ts doesn't turn into node/events because it's a special case global 👍🏻

Copy link
Member

@weswigham weswigham Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix seems incorrect and an odd special case. We expect in cases like this one, to use the module name specified by the ambient module declaration, not one from a filepath. Since the module is declared as declare module "events" we should only be using "events" to refer to it, and should never have looked at a filepath to begin with (notably: there's no guarantee the ambient module has the same name as the containing file!).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs to be reverted and correctly fixed as @weswigham points out correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh.... Sorry for the misleading...
And, I am a little confused, if there is not any special folder, according to the module resolution https://www.typescriptlang.org/docs/handbook/module-resolution.html. How could file find the correct modules? Did I miss something?

Copy link
Contributor Author

@ShuiRuTian ShuiRuTian Dec 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auh, I find this https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html#-reference-ing-a-module and finally understand why it is called "ambient" module......

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#41895 this one!

@orta
Copy link
Contributor

orta commented Dec 7, 2020

Thanks!

@orta orta merged commit 37e898c into microsoft:master Dec 7, 2020
scalder27 pushed a commit to scalder27/TypeScript that referenced this pull request Dec 7, 2020
@ShuiRuTian ShuiRuTian deleted the fix-41800 branch December 8, 2020 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated declaration for a JavaScript file refers to non-existing module
5 participants