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

Only filter ignored paths from module specifier generation if there exists a better option #43024

Merged
merged 3 commits into from Mar 2, 2021

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Mar 1, 2021

Fixes #42785
Fixes #43011

Sometimes the resolver can’t come up with a module specifier that doesn’t have an “ignored path” (like .pnpm or .prisma) in it. In those cases, we need to use the undesirable ignored path. In @dlannoye’s repro, the .pnpm path never actually manifests in a declaration file—the resolver separately finds an accessible re-export and uses that, but there was nevertheless an assertion that we could come up with something for the declaring module itself. The test case included is based off of @flybayer’s repro. This case does result in a type serialization that references node_modules/.prisma directly, which isn’t great, but that would have happened in 4.1 and earlier as well. (The real example from @flybayer has noEmit enabled, so in fact neither real-world case actually exhibits bad behavior with this PR. And I think for the Prisma case, it’s not strictly harmful if declaration files reference ".prisma" anyway, since that folder will have to get created during build/install for "@prisma/client" to work.)

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Is the assertion that triggered the crash still correct to have? Is this function no longer able to return undefined in the cases that feed into that assertion?

@DanielRosenwasser
Copy link
Member

Does this fix #43011 too? Looks potentially similar, mentions .pnpm.

@andrewbranch
Copy link
Member Author

Yes. Fixes #43011.

@andrewbranch
Copy link
Member Author

andrewbranch commented Mar 1, 2021

Is the assertion that triggered the crash still correct to have? Is this function no longer able to return undefined in the cases that feed into that assertion?

Yeah, the assertion is correct, and the function I changed is now once again guaranteed to call its callback at least once. (If we skip the filtering, targets always contains at least importedFileName and will run on that, unless better symlinks are found.)

@andrewbranch
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.2

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2021

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.2 on this PR at a687bae. You can monitor the build here.

@andrewbranch
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.2

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 1, 2021

Heya @andrewbranch, I've started to run the task to cherry-pick this into release-4.2 on this PR at 7597f52. You can monitor the build here.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 1, 2021
Component commits:
bd2fd3b Only filter ignored paths from module specifier generation if there exists a better option

a687bae Nit

7597f52 Merge branch 'master' into bug/42785
@typescript-bot
Copy link
Collaborator

Hey @andrewbranch, I've opened #43032 for you.

@andrewbranch andrewbranch merged commit 56f95d2 into microsoft:master Mar 2, 2021
@andrewbranch andrewbranch deleted the bug/42785 branch March 2, 2021 00:12
DanielRosenwasser pushed a commit that referenced this pull request Mar 2, 2021
Component commits:
bd2fd3b Only filter ignored paths from module specifier generation if there exists a better option

a687bae Nit

7597f52 Merge branch 'master' into bug/42785

Co-authored-by: Andrew Branch <andrew@wheream.io>
This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team 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.

Debug Failure Assertion violation: undefined relativeSpecifiers on Object.getModuleSpecifiers
5 participants