From 38bb2bfeb6829f47d84d71f4ffade7170f3331db Mon Sep 17 00:00:00 2001 From: tmpln Date: Fri, 8 May 2026 08:14:22 +0000 Subject: [PATCH] fix(core): improve input writes migration in best effort mode Currently, signal migration schematics in best effort mode doesn't do a very good job migrating input writes when there is a nested property access in templates. In event handlers, no attempt is made to migrate a nested access in the left-hand-side of assignments or anything in their right-hand-side. E.g., nothing will happen here: `(ngModelChange)="inputD.prop = $event + inputF"`. Additionally, when a migration attempt is made, parentheses are often incorrectly placed on the parent, both in event handlers and two-way bindings: `(ngModelChange)="inputC = $event"` is migrated to `(ngModelChange)="inputC = $event()"`. `[(ngModel)]="inputB.prop.prop"` is migrated to `[(ngModel)]="inputB.prop().prop"`. --- .../passes/7_migrate_template_references.ts | 10 +--------- .../src/passes/8_migrate_host_bindings.ts | 10 +--------- .../template_reference_visitor.ts | 16 ++++++++++------ .../test/golden-test/template_writes.ts | 14 +++++++++----- .../signal-migration/test/golden.txt | 18 +++++++++++++----- .../test/golden_best_effort.txt | 14 +++++++++----- 6 files changed, 43 insertions(+), 39 deletions(-) diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/7_migrate_template_references.ts b/packages/core/schematics/migrations/signal-migration/src/passes/7_migrate_template_references.ts index 18951efb18c3..b9f9c7c69aa5 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/7_migrate_template_references.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/7_migrate_template_references.ts @@ -30,15 +30,7 @@ export function pass7__migrateTemplateReferences continue; } - const parent = reference.from.readAstPath.at(-2); - let readEndPos: number; - - if (reference.from.isWrite && parent) { - readEndPos = parent.sourceSpan.end; - } else { - readEndPos = reference.from.read.sourceSpan.end; - } - + const readEndPos = reference.from.read.sourceSpan.end; // Skip duplicate references. E.g. if a template is shared. const fileReferenceId = `${reference.from.templateFile.id}:${readEndPos}`; if (seenFileReferences.has(fileReferenceId)) { diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/8_migrate_host_bindings.ts b/packages/core/schematics/migrations/signal-migration/src/passes/8_migrate_host_bindings.ts index f33181cb5ad6..64318db0e906 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/8_migrate_host_bindings.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/8_migrate_host_bindings.ts @@ -35,15 +35,7 @@ export function pass8__migrateHostBindings( const bindingField = reference.from.hostPropertyNode; const expressionOffset = bindingField.getStart() + 1; // account for quotes. - const parent = reference.from.readAstPath.at(-2); - let readEndPos: number; - - if (reference.from.isWrite && parent) { - readEndPos = expressionOffset + parent.sourceSpan.end; - } else { - readEndPos = expressionOffset + reference.from.read.sourceSpan.end; - } - + const readEndPos = expressionOffset + reference.from.read.sourceSpan.end; // Skip duplicate references. Can happen if the host object is shared. if (seenReferences.get(bindingField)?.has(readEndPos)) { continue; diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts index c22b6d2c0f28..73c2bc5b48dc 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/reference_resolution/template_reference_visitor.ts @@ -244,6 +244,7 @@ export class TemplateExpressionReferenceVisitor< private activeTmplAstNode: ExprContext | null = null; private detectedInputReferences: TmplInputExpressionReference[] = []; private isInsideObjectShorthandExpression = false; + private isInsideAssignment = false; private insideConditionalExpressionsWithReads: AST[] = []; constructor( @@ -285,17 +286,20 @@ export class TemplateExpressionReferenceVisitor< } override visitPropertyRead(ast: PropertyRead, context: AST[]) { - this._inspectPropertyAccess(ast, false, context); + this._inspectPropertyAccess(ast, context); super.visitPropertyRead(ast, context); } override visitSafePropertyRead(ast: SafePropertyRead, context: AST[]) { - this._inspectPropertyAccess(ast, false, context); + this._inspectPropertyAccess(ast, context); super.visitPropertyRead(ast, context); } override visitBinary(ast: Binary, context: AST[]) { - if (ast.operation === '=' && ast.left instanceof PropertyRead) { - this._inspectPropertyAccess(ast.left, true, [...context, ast, ast.left]); + if (ast.operation === '=') { + this.isInsideAssignment = true; + this.visit(ast.left, [...context, ast]); + this.isInsideAssignment = false; + this.visit(ast.right, [...context, ast]); } else { super.visitBinary(ast, context); } @@ -313,7 +317,7 @@ export class TemplateExpressionReferenceVisitor< * Inspects the property access and attempts to resolve whether they access * a known field. If so, the result is captured. */ - private _inspectPropertyAccess(ast: PropertyRead, isAssignment: boolean, astPath: AST[]) { + private _inspectPropertyAccess(ast: PropertyRead, astPath: AST[]) { if ( this.fieldNamesToConsiderForReferenceLookup !== null && !this.fieldNamesToConsiderForReferenceLookup.has(ast.name) @@ -322,7 +326,7 @@ export class TemplateExpressionReferenceVisitor< } const isWrite = !!( - isAssignment || + this.isInsideAssignment || (this.activeTmplAstNode && isTwoWayBindingNode(this.activeTmplAstNode)) ); diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/template_writes.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/template_writes.ts index a65753ea6768..f407401485a6 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden-test/template_writes.ts +++ b/packages/core/schematics/migrations/signal-migration/test/golden-test/template_writes.ts @@ -5,15 +5,19 @@ import {Component, Input} from '@angular/core'; @Component({ template: ` -
+ + + `, host: { - '(click)': 'inputC = true', + '(click)': 'inputE = true', }, }) class TwoWayBinding { @Input() inputA = ''; - @Input() inputB = true; - @Input() inputC = false; - @Input() inputD = false; + @Input() inputB = {prop: {prop: ''}}; + @Input() inputC = ''; + @Input() inputD = {prop: ''}; + @Input() inputE = false; + @Input() inputF = ''; } diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index aae75a001094..93040e090dde 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -1299,10 +1299,12 @@ import {Component, Input, input} from '@angular/core'; @Component({ template: ` -
+ + + `, host: { - '(click)': 'inputC = true', + '(click)': 'inputE = true', }, }) class TwoWayBinding { @@ -1311,11 +1313,17 @@ class TwoWayBinding { @Input() inputA = ''; // TODO: Skipped for migration because: // Your application code writes to the input. This prevents migration. - @Input() inputB = true; + @Input() inputB = {prop: {prop: ''}}; // TODO: Skipped for migration because: // Your application code writes to the input. This prevents migration. - @Input() inputC = false; - readonly inputD = input(false); + @Input() inputC = ''; + // TODO: Skipped for migration because: + // Your application code writes to the input. This prevents migration. + @Input() inputD = {prop: ''}; + // TODO: Skipped for migration because: + // Your application code writes to the input. This prevents migration. + @Input() inputE = false; + readonly inputF = input(''); } @@@@@@ temporary_variables.ts @@@@@@ diff --git a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt index aff417ca873b..a2752b9232ce 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt @@ -1253,17 +1253,21 @@ import {Component, input} from '@angular/core'; @Component({ template: ` -
+ + + `, host: { - '(click)': 'inputC = true()', + '(click)': 'inputE() = true', }, }) class TwoWayBinding { readonly inputA = input(''); - readonly inputB = input(true); - readonly inputC = input(false); - readonly inputD = input(false); + readonly inputB = input({ prop: { prop: '' } }); + readonly inputC = input(''); + readonly inputD = input({ prop: '' }); + readonly inputE = input(false); + readonly inputF = input(''); } @@@@@@ temporary_variables.ts @@@@@@