Skip to content

Avoid unnecessary resolution-mode assertion in declaration emit #55727

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 1 commit into from
Sep 13, 2023

Conversation

andrewbranch
Copy link
Member

Fixes the bug part of #55579, where the manual triple-slash reference workaround doesn’t work for @types/node.

When a type reference directive is processed, we also record what additional files the resolved types bring in via <reference path="..." /> directives. This way, when you reference a global declared in node_modules/@types/node/globals.d.ts, we can see that it actually came in via a <reference types="node" />. But instead of recording this entry with the resolution-mode that the types reference had, we recorded it with the resolution-mode that the file reference had, which is irrelevant (and could even lead to some totally incorrect results with some manufactured bizarre cases). All we need to know is how did this file enter the program? which is the type reference directive with whatever mode it had.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 13, 2023
@andrewbranch
Copy link
Member Author

@mpodwysocki I was never able to repro a case where the declaration emitter included a duplicate triple-slash directive, so I can’t be sure whether this fixes the whole problem. Would you mind trying this build out on your repro?

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 49f8c32. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/157676/artifacts?artifactName=tgz&fileId=7D92E271BFAD4CCDDEED93C285D7F869BF417D6FF114019F46959151848DB9CA02&fileName=/typescript-5.3.0-insiders.20230913.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@mpodwysocki
Copy link
Member

@mpodwysocki I was never able to repro a case where the declaration emitter included a duplicate triple-slash directive, so I can’t be sure whether this fixes the whole problem. Would you mind trying this build out on your repro?

@typescript-bot pack this

@andrewbranch it works perfectly, thank you!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

Heya @mpodwysocki, I've started to run the tarball bundle task on this PR at 49f8c32. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2023

Hey @mpodwysocki, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/157693/artifacts?artifactName=tgz&fileId=49DD2CFB19567947EF8097C9BCC217E816045571BFEB8783F844333DC4DA70AB02&fileName=/typescript-5.3.0-insiders.20230913.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@andrewbranch andrewbranch merged commit 07bca99 into main Sep 13, 2023
@andrewbranch andrewbranch deleted the bug/dts-resolution-mode branch September 13, 2023 20:31
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants