Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 648aad2

Browse filesBrowse files
devversionthePunderWoman
devversion
authored and
thePunderWoman
committed
refactor: ensure tsurge migrations have clear ownership of files (#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 #61421
1 parent e409d94 commit 648aad2
Copy full SHA for 648aad2

File tree

Expand file treeCollapse file tree

19 files changed

+156
-89
lines changed
Filter options
Expand file treeCollapse file tree

19 files changed

+156
-89
lines changed

‎packages/core/schematics/migrations/signal-migration/src/cli.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/migrations/signal-migration/src/cli.ts
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import path from 'path';
1111
import assert from 'assert';
1212
import {SignalInputMigration} from './migration';
1313
import {writeMigrationReplacements} from './write_replacements';
14+
import {NodeJSFileSystem} from '../../../../../compiler-cli';
1415

1516
main(
1617
path.resolve(process.argv[2]),
@@ -34,8 +35,7 @@ export async function main(
3435
insertTodosForSkippedFields,
3536
upgradeAnalysisPhaseToAvoidBatch: true,
3637
});
37-
const baseInfo = migration.createProgram(absoluteTsconfigPath);
38-
const info = migration.prepareProgram(baseInfo);
38+
const info = migration.createProgram(absoluteTsconfigPath, new NodeJSFileSystem());
3939

4040
await migration.analyze(info);
4141

‎packages/core/schematics/migrations/signal-migration/src/migration.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/migrations/signal-migration/src/migration.ts
+13-9Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,12 @@ import {Replacement} from '../../../utils/tsurge/replacement';
2323
import {populateKnownInputsFromGlobalData} from './batch/populate_global_data';
2424
import {executeMigrationPhase} from './phase_migrate';
2525
import {filterIncompatibilitiesForBestEffortMode} from './best_effort_mode';
26-
import assert from 'assert';
2726
import {
2827
ClassIncompatibilityReason,
2928
FieldIncompatibilityReason,
3029
} from './passes/problematic_patterns/incompatibility';
3130
import {MigrationConfig} from './migration_config';
3231
import {ClassFieldUniqueKey} from './passes/reference_resolution/known_fields';
33-
import {createBaseProgramInfo} from '../../../utils/tsurge/helpers/create_program';
3432

3533
/**
3634
* Tsurge migration for migrating Angular `@Input()` declarations to
@@ -50,9 +48,8 @@ export class SignalInputMigration extends TsurgeComplexMigration<
5048
super();
5149
}
5250

53-
// Override the default program creation, to add extra flags.
54-
override createProgram(tsconfigAbsPath: string, fs?: FileSystem): BaseProgramInfo {
55-
return createBaseProgramInfo(tsconfigAbsPath, fs, {
51+
override createProgram(tsconfigAbsPath: string, fs: FileSystem): ProgramInfo {
52+
return super.createProgram(tsconfigAbsPath, fs, {
5653
_compilePoisonedComponents: true,
5754
// We want to migrate non-exported classes too.
5855
compileNonExportedClasses: true,
@@ -63,8 +60,11 @@ export class SignalInputMigration extends TsurgeComplexMigration<
6360
});
6461
}
6562

66-
override prepareProgram(baseInfo: BaseProgramInfo): ProgramInfo {
67-
const info = super.prepareProgram(baseInfo);
63+
/**
64+
* Prepares the program for this migration with additional custom
65+
* fields to allow for batch-mode testing.
66+
*/
67+
private _prepareProgram(info: ProgramInfo): ProgramInfo {
6868
// Optional filter for testing. Allows for simulation of parallel execution
6969
// even if some tsconfig's have overlap due to sharing of TS sources.
7070
// (this is commonly not the case in g3 where deps are `.d.ts` files).
@@ -74,7 +74,7 @@ export class SignalInputMigration extends TsurgeComplexMigration<
7474
// Optional replacement filter. Allows parallel execution in case
7575
// some tsconfig's have overlap due to sharing of TS sources.
7676
// (this is commonly not the case in g3 where deps are `.d.ts` files).
77-
!limitToRootNamesOnly || info.programAbsoluteRootFileNames!.includes(f.fileName),
77+
!limitToRootNamesOnly || info.__programAbsoluteRootFileNames!.includes(f.fileName),
7878
);
7979

8080
return {
@@ -87,12 +87,14 @@ export class SignalInputMigration extends TsurgeComplexMigration<
8787
prepareAnalysisDeps(info: ProgramInfo): AnalysisProgramInfo {
8888
const analysisInfo = {
8989
...info,
90-
...prepareAnalysisInfo(info.program, info.ngCompiler, info.programAbsoluteRootFileNames),
90+
...prepareAnalysisInfo(info.program, info.ngCompiler, info.__programAbsoluteRootFileNames),
9191
};
9292
return analysisInfo;
9393
}
9494

9595
override async analyze(info: ProgramInfo) {
96+
info = this._prepareProgram(info);
97+
9698
const analysisDeps = this.prepareAnalysisDeps(info);
9799
const knownInputs = new KnownInputs(info, this.config);
98100
const result = new MigrationResult();
@@ -163,6 +165,8 @@ export class SignalInputMigration extends TsurgeComplexMigration<
163165
analysisDeps: AnalysisProgramInfo;
164166
},
165167
) {
168+
info = this._prepareProgram(info);
169+
166170
const knownInputs = nonBatchData?.knownInputs ?? new KnownInputs(info, this.config);
167171
const result = nonBatchData?.result ?? new MigrationResult();
168172
const host = nonBatchData?.host ?? createMigrationHost(info, this.config);

‎packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/cleanup-unused-imports/unused_imports_migration.ts
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
import ts from 'typescript';
1010
import {
11-
BaseProgramInfo,
1211
confirmAsSerializable,
1312
ProgramInfo,
1413
projectFile,
@@ -19,7 +18,7 @@ import {
1918
TsurgeFunnelMigration,
2019
} from '../../utils/tsurge';
2120
import {ErrorCode, FileSystem, ngErrorCode} from '@angular/compiler-cli';
22-
import {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api';
21+
import {DiagnosticCategoryLabel, NgCompilerOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
2322
import {ImportManager} from '@angular/compiler-cli/private/migrations';
2423
import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import_manager';
2524

@@ -70,7 +69,7 @@ export class UnusedImportsMigration extends TsurgeFunnelMigration<
7069
> {
7170
private printer = ts.createPrinter();
7271

73-
override createProgram(tsconfigAbsPath: string, fs?: FileSystem): BaseProgramInfo {
72+
override createProgram(tsconfigAbsPath: string, fs: FileSystem): ProgramInfo {
7473
return super.createProgram(tsconfigAbsPath, fs, {
7574
extendedDiagnostics: {
7675
checks: {

‎packages/core/schematics/utils/tsurge/base_migration.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/base_migration.ts
+7-41Lines changed: 7 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import {absoluteFrom, FileSystem} from '@angular/compiler-cli/src/ngtsc/file_sys
1010
import {NgCompilerOptions} from '@angular/compiler-cli/src/ngtsc/core/api';
1111
import {getRootDirs} from '@angular/compiler-cli/src/ngtsc/util/src/typescript';
1212
import {isShim} from '@angular/compiler-cli/src/ngtsc/shims';
13-
import {BaseProgramInfo, ProgramInfo} from './program_info';
13+
import {ProgramInfo} from './program_info';
1414
import {Serializable} from './helpers/serializable';
15-
import {createBaseProgramInfo} from './helpers/create_program';
15+
import {createBaseProgramInfo, getProgramInfoFromBaseInfo} from './helpers/create_program';
1616

1717
/** Type helper extracting the stats type of a migration. */
1818
export type MigrationStats<T> =
@@ -33,52 +33,18 @@ export abstract class TsurgeBaseMigration<
3333
Stats = unknown,
3434
> {
3535
/**
36-
* Advanced Tsurge users can override this method, but most of the time,
37-
* overriding {@link prepareProgram} is more desirable.
36+
* Creates the TypeScript program for a given compilation unit.
3837
*
3938
* By default:
4039
* - In 3P: Ngtsc programs are being created.
4140
* - In 1P: Ngtsc or TS programs are created based on the Blaze target.
4241
*/
4342
createProgram(
4443
tsconfigAbsPath: string,
45-
fs?: FileSystem,
46-
optionOverrides?: NgCompilerOptions,
47-
): BaseProgramInfo {
48-
return createBaseProgramInfo(tsconfigAbsPath, fs, optionOverrides);
49-
}
50-
51-
// Optional function to prepare the base `ProgramInfo` even further,
52-
// for the analyze and migrate phases. E.g. determining source files.
53-
prepareProgram(info: BaseProgramInfo): ProgramInfo {
54-
const fullProgramSourceFiles = [...info.program.getSourceFiles()];
55-
const sourceFiles = fullProgramSourceFiles.filter(
56-
(f) =>
57-
!f.isDeclarationFile &&
58-
// Note `isShim` will work for the initial program, but for TCB programs, the shims are no longer annotated.
59-
!isShim(f) &&
60-
!f.fileName.endsWith('.ngtypecheck.ts'),
61-
);
62-
63-
// Sort it by length in reverse order (longest first). This speeds up lookups,
64-
// since there's no need to keep going through the array once a match is found.
65-
const sortedRootDirs = getRootDirs(info.host, info.userOptions).sort(
66-
(a, b) => b.length - a.length,
67-
);
68-
69-
// TODO: Consider also following TS's logic here, finding the common source root.
70-
// See: Program#getCommonSourceDirectory.
71-
const primaryRoot = absoluteFrom(
72-
info.userOptions.rootDir ?? sortedRootDirs.at(-1) ?? info.program.getCurrentDirectory(),
73-
);
74-
75-
return {
76-
...info,
77-
sourceFiles,
78-
fullProgramSourceFiles,
79-
sortedRootDirs,
80-
projectRoot: primaryRoot,
81-
};
44+
fs: FileSystem,
45+
optionsOverride?: NgCompilerOptions,
46+
): ProgramInfo {
47+
return getProgramInfoFromBaseInfo(createBaseProgramInfo(tsconfigAbsPath, fs, optionsOverride));
8248
}
8349

8450
/** Analyzes the given TypeScript project and returns serializable compilation unit data. */

‎packages/core/schematics/utils/tsurge/executors/analyze_exec.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/executors/analyze_exec.ts
+3-3Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9+
import {NodeJSFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
910
import {TsurgeMigration} from '../migration';
1011
import {Serializable} from '../helpers/serializable';
1112

1213
/**
13-
* Executes the analyze phase of the given migration against
14+
* 1P Logic: Executes the analyze phase of the given migration against
1415
* the specified TypeScript project.
1516
*
1617
* @returns the serializable migration unit data.
@@ -19,8 +20,7 @@ export async function executeAnalyzePhase<UnitData, GlobalData>(
1920
migration: TsurgeMigration<UnitData, GlobalData, unknown>,
2021
tsconfigAbsolutePath: string,
2122
): Promise<Serializable<UnitData>> {
22-
const baseInfo = migration.createProgram(tsconfigAbsolutePath);
23-
const info = migration.prepareProgram(baseInfo);
23+
const info = migration.createProgram(tsconfigAbsolutePath, new NodeJSFileSystem());
2424

2525
return await migration.analyze(info);
2626
}

‎packages/core/schematics/utils/tsurge/executors/combine_exec.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/executors/combine_exec.ts
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {Serializable} from '../helpers/serializable';
1010
import {TsurgeMigration} from '../migration';
1111

1212
/**
13-
* Executes the combine phase for the given migration against
13+
* 1P Logic: Executes the combine phase for the given migration against
1414
* two unit analyses.
1515
*
1616
* @returns the serializable combined unit data.

‎packages/core/schematics/utils/tsurge/executors/global_meta_exec.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/executors/global_meta_exec.ts
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {Serializable} from '../helpers/serializable';
1010
import {TsurgeMigration} from '../migration';
1111

1212
/**
13-
* Executes the `globalMeta` stage for the given migration
13+
* 1P Logic: Executes the `globalMeta` stage for the given migration
1414
* to convert the combined unit data into global meta.
1515
*
1616
* @returns the serializable global meta.

‎packages/core/schematics/utils/tsurge/executors/migrate_exec.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/executors/migrate_exec.ts
+4-4Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,12 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/file_system';
9+
import {AbsoluteFsPath, NodeJSFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
1010
import {TsurgeMigration} from '../migration';
1111
import {Replacement} from '../replacement';
1212

1313
/**
14-
* Executes the migrate phase of the given migration against
14+
* 1P Logic: Executes the migrate phase of the given migration against
1515
* the specified TypeScript project.
1616
*
1717
* This requires the global migration data, computed by the
@@ -25,8 +25,8 @@ export async function executeMigratePhase<UnitData, GlobalData>(
2525
globalMetadata: GlobalData,
2626
tsconfigAbsolutePath: string,
2727
): Promise<{replacements: Replacement[]; projectRoot: AbsoluteFsPath}> {
28-
const baseInfo = migration.createProgram(tsconfigAbsolutePath);
29-
const info = migration.prepareProgram(baseInfo);
28+
// In 1P, we never need to use a virtual file system.
29+
const info = migration.createProgram(tsconfigAbsolutePath, new NodeJSFileSystem());
3030

3131
return {
3232
...(await migration.migrate(globalMetadata, info)),

‎packages/core/schematics/utils/tsurge/helpers/angular_devkit/BUILD.bazel

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/helpers/angular_devkit/BUILD.bazel
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,5 +13,6 @@ ts_project(
1313
deps = [
1414
"//:node_modules/@angular-devkit/core",
1515
"//:node_modules/@angular-devkit/schematics",
16+
"//:node_modules/typescript",
1617
],
1718
)

‎packages/core/schematics/utils/tsurge/helpers/angular_devkit/run_in_devkit.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/utils/tsurge/helpers/angular_devkit/run_in_devkit.ts
+45-6Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,11 @@ import {DevkitMigrationFilesystem} from './devkit_filesystem';
1212
import {groupReplacementsByFile} from '../group_replacements';
1313
import {synchronouslyCombineUnitData} from '../combine_units';
1414
import {TsurgeFunnelMigration, TsurgeMigration} from '../../migration';
15-
import {MigrationStats} from '../../base_migration';
1615
import {Replacement, TextUpdate} from '../../replacement';
1716
import {ProjectRootRelativePath} from '../../project_paths';
1817
import {ProgramInfo} from '../../program_info';
1918
import {getProjectTsConfigPaths} from '../../../../utils/project_tsconfig_paths';
19+
import ts from 'typescript';
2020

2121
export enum MigrationStage {
2222
/** The migration is analyzing an entrypoint */
@@ -74,13 +74,17 @@ export async function runMigrationInDevkit<Stats>(
7474
const unitResults: unknown[] = [];
7575

7676
const isFunnelMigration = migration instanceof TsurgeFunnelMigration;
77+
const compilationUnitAssignments = new Map<string, string>();
78+
7779
for (const tsconfigPath of tsconfigPaths) {
7880
config.beforeProgramCreation?.(tsconfigPath, MigrationStage.Analysis);
79-
const baseInfo = migration.createProgram(tsconfigPath, fs);
80-
const info = migration.prepareProgram(baseInfo);
81-
config.afterProgramCreation?.(info, fs, MigrationStage.Analysis);
81+
const info = migration.createProgram(tsconfigPath, fs);
8282

83+
modifyProgramInfoToEnsureNonOverlappingFiles(tsconfigPath, info, compilationUnitAssignments);
84+
85+
config.afterProgramCreation?.(info, fs, MigrationStage.Analysis);
8386
config.beforeUnitAnalysis?.(tsconfigPath);
87+
8488
unitResults.push(await migration.analyze(info));
8589
}
8690

@@ -102,8 +106,9 @@ export async function runMigrationInDevkit<Stats>(
102106

103107
for (const tsconfigPath of tsconfigPaths) {
104108
config.beforeProgramCreation?.(tsconfigPath, MigrationStage.Migrate);
105-
const baseInfo = migration.createProgram(tsconfigPath, fs);
106-
const info = migration.prepareProgram(baseInfo);
109+
const info = migration.createProgram(tsconfigPath, fs);
110+
modifyProgramInfoToEnsureNonOverlappingFiles(tsconfigPath, info, compilationUnitAssignments);
111+
107112
config.afterProgramCreation?.(info, fs, MigrationStage.Migrate);
108113

109114
const result = await migration.migrate(globalMeta, info);
@@ -132,3 +137,37 @@ export async function runMigrationInDevkit<Stats>(
132137

133138
config.whenDone?.(await migration.stats(globalMeta));
134139
}
140+
141+
/**
142+
* Special logic for devkit migrations. In the Angular CLI, or in 3P precisely,
143+
* projects can have tsconfigs with overlapping source files. i.e. two tsconfigs
144+
* like e.g. build or test include the same `ts.SourceFile` (`.ts`). Migrations
145+
* should never have 2+ compilation units with overlapping source files as this
146+
* can result in duplicated replacements or analysis— hence we only ever assign a
147+
* source file to a compilation unit *once*.
148+
*
149+
* Note that this is fine as we expect Tsurge migrations to work together as
150+
* isolated compilation units— so it shouldn't matter if worst case a `.ts`
151+
* file ends up in the e.g. test program.
152+
*/
153+
function modifyProgramInfoToEnsureNonOverlappingFiles(
154+
tsconfigPath: string,
155+
info: ProgramInfo,
156+
compilationUnitAssignments: Map<string, string>,
157+
) {
158+
const sourceFiles: ts.SourceFile[] = [];
159+
160+
for (const sf of info.sourceFiles) {
161+
const assignment = compilationUnitAssignments.get(sf.fileName);
162+
163+
// File is already assigned to a different compilation unit.
164+
if (assignment !== undefined && assignment !== tsconfigPath) {
165+
continue;
166+
}
167+
168+
compilationUnitAssignments.set(sf.fileName, tsconfigPath);
169+
sourceFiles.push(sf);
170+
}
171+
172+
info.sourceFiles = sourceFiles;
173+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.