Skip to content

Don't add ambiently declared modules to ATA's unresolvedModules list#20464

Merged
RyanCavanaugh merged 1 commit into
microsoft:masterfrom
RyanCavanaugh:fix20402
Dec 6, 2017
Merged

Don't add ambiently declared modules to ATA's unresolvedModules list#20464
RyanCavanaugh merged 1 commit into
microsoft:masterfrom
RyanCavanaugh:fix20402

Conversation

@RyanCavanaugh
Copy link
Copy Markdown
Member

Fixes #20402

Builds on #20403; only the last commit is new relative to that one

@RyanCavanaugh
Copy link
Copy Markdown
Member Author

N.B. from issue

Fix is to tell ATA to not ask the typings installer to add types that are part of the implicit type references of the project

This turns out to be insufficient - even after excluding node (because it's part of the implicit type references of the project), we go add it again later in the typingsInstaller because we see e.g. fs in the unresolvedImports of one of the local @types/node's .d.ts files, and helpfully map the fs module to node. Note that this mapping occurs in the typingsInstaller which doesn't even know that the node file is the types package for node.

The prior fix in #20403 is sufficient in cases where an ambient module is declared in the location where we would find it if it were a proper module, but doesn't fix the case where it's simply ambient in a random location. The simple additional fix here (which doesn't per se exclude the other one, because the other fix improves performance) is to ask for the list of ambient modules from the checker and exclude them.

Comment thread src/compiler/program.ts Outdated
const resolutionToFile = getResolvedModule(oldProgramState.oldSourceFile, moduleName);
const resolvedFile = resolutionToFile && oldProgramState.program && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName);
if (resolutionToFile && !resolvedFile.externalModuleIndicator) {
// module used to be resolved to module - ignore it
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment here says that the module use to be resolved to module and yet we check !resolvedFile.externalModuleIndicator?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is for the case where we previously resolved to a module that ultimately contained an ambient declaration for the module we were looking for, but isn't actually itself the module. In that case we can't "re-use" the resolution.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That makes sense, can you please update the comment.

@RyanCavanaugh RyanCavanaugh requested a review from billti December 5, 2017 21:47
Comment thread src/server/project.ts
abstract getTypeAcquisition(): TypeAcquisition;

protected removeLocalTypingsFromTypeAcquisition(newTypeAcquisition: TypeAcquisition): TypeAcquisition {
if (!newTypeAcquisition || !newTypeAcquisition.include) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this right? if (!something) return something always reads a bit fishy.

@RyanCavanaugh RyanCavanaugh merged commit ee283d1 into microsoft:master Dec 6, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants