refactor: ensure tsurge migrations have clear ownership of files #61421
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
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).
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.
…hen cleaning up unused imports (angular#59656)" This reverts commit d66881d.
crisbeto
approved these changes
May 16, 2025
thePunderWoman
pushed a commit
that referenced
this pull request
May 16, 2025
) 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
thePunderWoman
pushed a commit
that referenced
this pull request
May 16, 2025
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 #61421
thePunderWoman
pushed a commit
that referenced
this pull request
May 16, 2025
thePunderWoman
pushed a commit
that referenced
this pull request
May 16, 2025
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 #61421
thePunderWoman
pushed a commit
that referenced
this pull request
May 16, 2025
|
This PR was merged into the repository by commit e4d2347. The changes were merged into the following branches: main, 20.0.x |
1 task
JeanMeche
pushed a commit
to JeanMeche/angular
that referenced
this pull request
May 22, 2025
…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
JeanMeche
pushed a commit
to JeanMeche/angular
that referenced
this pull request
May 22, 2025
…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
JeanMeche
added a commit
to JeanMeche/angular
that referenced
this pull request
May 22, 2025
This is a patch port of angular#61421
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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Currently there can be cases, exclusively 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
.tssource filecan 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.tsendsup being part of the test tsconfig compilation unit (we avoid this order
though by visiting build targets first).
Related #61337