refactor: ensure tsurge migrations have clear ownership of files #61612
Conversation
This file contains hidden or 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
…ular#61421) Currently there can be cases, exlusively in 3P, where multiple tsconfig projects have overlap of source files. This is the default setup of new CLI applications as well. When this is the case, Tsurge will treat each tsconfig as an isolated compilation unit (given the concepts and mental model to support scalable batching). This is wrong though, and the same `.ts` source file can appear in two migration invocations; resulting in duplicate replacements or analysis (depending on the migration). We've worked around this problem in the past by deduplicating replacements, or migrating to an ID-based approach with natural deduplication. This worked, but it's just working around the root cause. This commit attempts to fix the root cause by adjusting Tsurge to ensure that no source file ever appears in two compilation units. This is naively achieved by not adding a source file to a migration unit, if it was part of a previous one. This is expected to be fine given the nature of Tsurge migrations that are built to operate on isolated pieces anyway— so it shouldn't be problematic if e.g. `app.component.ts` ends up being part of the test tsconfig compilation unit (we avoid this order though by visiting build targets first). PR Close angular#61421
…61421) Since the duplication root-cause was solved by the previous commit, we can revert/drop the logic that was added back then to overcome this problem with Tsurge. PR Close angular#61421
1 task
This is a patch port of angular#61421
e2d169f to
3628a3d
Compare
devversion
approved these changes
May 22, 2025
|
Thanks @JeanMeche! |
|
This PR was merged into the repository by commit 2ae69f7. The changes were merged into the following branches: 19.2.x |
atscott
pushed a commit
that referenced
this pull request
May 22, 2025
) (#61612) Currently there can be cases, exlusively in 3P, where multiple tsconfig projects have overlap of source files. This is the default setup of new CLI applications as well. When this is the case, Tsurge will treat each tsconfig as an isolated compilation unit (given the concepts and mental model to support scalable batching). This is wrong though, and the same `.ts` source file can appear in two migration invocations; resulting in duplicate replacements or analysis (depending on the migration). We've worked around this problem in the past by deduplicating replacements, or migrating to an ID-based approach with natural deduplication. This worked, but it's just working around the root cause. This commit attempts to fix the root cause by adjusting Tsurge to ensure that no source file ever appears in two compilation units. This is naively achieved by not adding a source file to a migration unit, if it was part of a previous one. This is expected to be fine given the nature of Tsurge migrations that are built to operate on isolated pieces anyway— so it shouldn't be problematic if e.g. `app.component.ts` ends up being part of the test tsconfig compilation unit (we avoid this order though by visiting build targets first). PR Close #61421 PR Close #61612
atscott
pushed a commit
that referenced
this pull request
May 22, 2025
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 is a patch port of #61421