-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Set impliedNodeFormat based on redirectedReference options #60039
base: main
Are you sure you want to change the base?
Conversation
tests/baselines/reference/tscWatch/projectsWithReferences/on-sample-project-with-nodenext.js
Outdated
Show resolved
Hide resolved
@typescript-bot test it |
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
@andrewbranch Here are the results of running the user tests with tsc comparing Everything looks good! |
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@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 test it |
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
@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 @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
@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.
@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
@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 pack this |
Hey @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