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.)

@andrewbranch andrewbranch force-pushed the andrewbranch:bug/42785 branch from e36839a to bd2fd3b Mar 1, 2021
Copy link
Member

@weswigham weswigham left a comment

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

@DanielRosenwasser DanielRosenwasser commented Mar 1, 2021

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

@andrewbranch
Copy link
Member Author

@andrewbranch andrewbranch commented Mar 1, 2021

Yes. Fixes #43011.

@andrewbranch
Copy link
Member Author

@andrewbranch 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

@andrewbranch andrewbranch commented Mar 1, 2021

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

@typescript-bot
Copy link
Collaborator

@typescript-bot 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

@andrewbranch andrewbranch commented Mar 1, 2021

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

@typescript-bot
Copy link
Collaborator

@typescript-bot 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

@typescript-bot typescript-bot commented Mar 1, 2021

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

@andrewbranch andrewbranch merged commit 56f95d2 into microsoft:master Mar 2, 2021
9 checks passed
9 checks passed
@github-actions
build (10.x)
Details
@github-actions
CodeQL-Build CodeQL-Build
Details
@github-actions
build (12.x)
Details
@github-actions
build (14.x)
Details
@github-code-scanning
CodeQL No new or fixed alerts
Details
@microsoft-cla
license/cla All CLA requirements met.
Details
@azure-pipelines
node10 Build #97278 succeeded
Details
@azure-pipelines
node12 Build #97276 succeeded
Details
@azure-pipelines
node14 Build #97277 succeeded
Details
@andrewbranch andrewbranch deleted the andrewbranch:bug/42785 branch Mar 2, 2021
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants