-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
For duplicate source files of the same package, make one redirect to the other #16274
Merged
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
60b9aa3
For duplicate source files of the same package, make one redirect to …
8157308
Add reuseProgramStructure tests
e4d5f0d
Copy `sourceFileToPackageId` and `isSourceFileTargetOfRedirect` only …
17c5619
Merge branch 'master' into package-id
1f32e62
Use fallthrough instead of early exit from loop
f66713d
Use a set to efficiently detect duplicate package names
04cae2e
Move map setting outside of createRedirectSourceFile
61b768a
Correctly handle seenPackageNames set
4d55f34
sourceFileToPackageId -> sourceFileToPackageName
d702180
Merge branch 'master' into package-id
e2a0931
Merge branch 'master' into package-id
09e1c92
Renames
d557e9a
Merge branch 'master' into package-id
3d2fc00
Respond to PR comments
018c9c5
Fix bug where `oldSourceFile !== newSourceFile` because oldSourceFile…
91b2575
Clean up redirectInfo
bec2d03
Merge branch 'master' into package-id
8408b3d
Respond to PR comments
9f74f55
Merge branch 'master' into package-id
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2330,12 +2330,12 @@ namespace ts { | |
*/ | ||
/* @internal */ redirect?: { | ||
/** Source file this redirects to. */ | ||
readonly redirectTo: SourceFile, | ||
readonly redirectTarget: SourceFile, | ||
/** | ||
* Source file for the duplicate package. This will not be used by the Program, | ||
* but we need to keep this around so we can watch for changes in underlying. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to update the comments that still refer to "underlying". |
||
*/ | ||
readonly underlying: SourceFile, | ||
readonly unredirected: SourceFile, | ||
} | undefined; | ||
|
||
amdDependencies: AmdDependency[]; | ||
|
@@ -2511,8 +2511,8 @@ namespace ts { | |
|
||
/** Given a source file, get the name of the package it was imported from. */ | ||
/* @internal */ sourceFileToPackageName: Map<string>; | ||
/** True if some other source file redirects to this one. */ | ||
/* @internal */ isSourceFileTargetOfRedirect: Map<true>; | ||
/** Set of all source files that some other source file redirects to. */ | ||
/* @internal */ redirectTargetsSet: Map<true>; | ||
} | ||
|
||
/* @internal */ | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,7 @@ namespace ts { | |
const files = arrayToMap(texts, t => t.name, t => { | ||
if (oldProgram) { | ||
let oldFile = <SourceFileWithText>oldProgram.getSourceFile(t.name); | ||
if (oldFile && oldFile.redirect) oldFile = oldFile.redirect.underlying; | ||
if (oldFile && oldFile.redirect) oldFile = oldFile.redirect.unredirected; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use braces. |
||
if (oldFile && oldFile.sourceText.getVersion() === t.text.getVersion()) { | ||
return oldFile; | ||
} | ||
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SafeModules
isn't conservative enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, because we haven't set the dirty bit on the affected projects? Couldn't we? We're watching the files anyway, aren't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, we use
StructureIsReused.Not
in situations where files are added or removed, which would include the case of redirects being broken or created.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our offline discussion, I agree that this is an appropriate way to handle changes.