Stop checking existing resolution now that we have concept of package ID #58528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was added in #7775 when we didnt have deduplication logic So now just rely on the existing dedup logic for type Ref management.
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Starting jobs; this comment will be updated as builds start and complete.
|
|
Hey @sheetalkamat, 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 |
|
Hey @sheetalkamat, the results of running the DT tests are ready. Everything looks the same! |
|
@sheetalkamat Here are the results of running the user tests comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@sheetalkamat Here are the results of running the user tests comparing Everything looks good! |
|
@sheetalkamat Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sheetalkamat Here are the results of running the top 400 repos comparing Everything looks good! |
sheetalkamat
commented
May 14, 2024
| @@ -12,10 +12,14 @@ declare var foo: any; | |||
|
|
|||
| === /node_modules/foo/node_modules/alpha/index.d.ts === | |||
| declare var alpha: any; | |||
| >alpha : Symbol(alpha, Decl(index.d.ts, 0, 11)) | |||
| >alpha : Symbol(alpha, Decl(index.d.ts, 0, 11), Decl(index.d.ts, 0, 11)) | |||
There was a problem hiding this comment.
These didnt have package.json so didnt get deduped. But I added test case 4b and 5b where there is package.json
jakebailey
approved these changes
May 14, 2024
Comment on lines
+19
to
+23
| ==== /node_modules/bar/node_modules/alpha/index.d.ts (1 errors) ==== | ||
| declare var alpha: {}; | ||
| ~~~~~ | ||
| !!! error TS2403: Subsequent variable declarations must have the same type. Variable 'alpha' must be of type 'any', but here has type '{}'. | ||
| !!! related TS6203 /node_modules/foo/node_modules/alpha/index.d.ts:1:13: 'alpha' was also declared here. |
There was a problem hiding this comment.
I was going to say "the old message seemed useful because it said where the dupe was", but I guess this related line would be enough. Though it's moderately a shame to lose a message that gives a potential solution.
There was a problem hiding this comment.
The things work much better where you could have two typings that dont add to global and used by two packages and hence two different versions. This handles that well. We have this issue with module resolution. The only message we gave was with typeRef and it wasnt correct checking either so i feel better removing this custom logic for typeRef only and allows packages to use typeRef of different versions if they dont add conflicting global info.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was added in #7775 when we didnt have deduplication logic So now just rely on the existing dedup logic for type Ref management.
This now follows exact same logic of module resolution and let the deduping take care of it. It also solves issue of when and how the type ref was resolved when they are same etc. Note that module resolution falls back to typeRef resolution on failure so right now you could still have a types in your program from two different versions. So this just streamlines it.
The test cases dont have package json and hence those are not deduped