Set impliedNodeFormat based on redirectedReference options - #60039
Set impliedNodeFormat based on redirectedReference options#60039Andrew Branch (andrewbranch) wants to merge 19 commits into
Conversation
|
TypeScript Bot (@typescript-bot) test it |
|
Hey Andrew Branch (@andrewbranch), the results of running the DT tests are ready. Everything looks the same! |
|
Andrew Branch (@andrewbranch) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Andrew Branch (@andrewbranch) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Andrew Branch (@andrewbranch) Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
|
I have a strong suspicion that redirected references and I'm still trying to reduce that repro case but I figured out it might be an interesting info to you. I have confirmed that this PR doesn't fix that issue right now. |
|
I think our strategy here is going to have to change significantly. The fix in this PR as it stands breaks the scenario tested in 100f939. We can’t actually skip setting |
|
I feel like this value should be in program instead of on sourceFile because sourceFiles are shared across programs. Eg thats what we did for module resolution as it causes issues otherwise. Otherwise sourceFile with given options "OptionsAffectingSourceFile" need to handle these cases and then it starts depending on ModuleResolution options again? (we removed that dependency) |
|
That’s one option I thought about. |
|
I can confirm that this now fixes #60029 (comment) . I'm still working on creating a test based on that crash though. |
|
TypeScript Bot (@typescript-bot) test it |
|
Hey Andrew Branch (@andrewbranch), the results of running the DT tests are ready. Everything looks the same! |
|
Andrew Branch (@andrewbranch) Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up. |
|
Andrew Branch (@andrewbranch) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Pretty sure what I have works, though moving the storage to Program would be more efficient, though also a public API break... I don’t think I’ll have time to get this into 5.7 as I’ll be out on vacation starting tomorrow and returning past the beta deadline. |
| || fileExtensionIsOneOf(sourceFile.fileName, [Extension.Cjs, Extension.Cts])) | ||
| ModuleKind.Node16 <= moduleKind && moduleKind <= ModuleKind.NodeNext | ||
| || fileExtensionIsOneOf(sourceFile.fileName, [Extension.Cts, Extension.Dcts, Extension.Cjs, Extension.Mts, Extension.Dmts, Extension.Mjs]) | ||
| || pathContainsNodeModules(sourceFile.fileName) |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Andrew Branch (@andrewbranch) Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
… `impliedNodeFormat` clash
|
It took me a while to reduce this 🥵 but here's a test case for the crash I mentioned in my previous comments here: andrewbranch#3 |
…e-format-and-document-registry Add extra test case for a weird situation with symlinks and potential `impliedNodeFormat` clash
|
Jake Bailey (@jakebailey) could you build this one? :) a user would like to test effect of this PR on the VS Code crashes they experience: #60243 (comment) |
|
Unfortunately not, as the PR has merge conflicts. |
Bug/57553 resolve conflicts
|
TypeScript Bot (@typescript-bot) pack this |
|
Hey Andrew Branch (@andrewbranch), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Fixes #57553