Elgato_dark/angular
1
0
Fork 0
mirror of https://github.com/angular/angular synced 2026-05-24 09:28:37 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

refactor(language-service): improve error messaging for signal input refactoring (#57659)

Browse source 
Instead of printing the enum name as the reason why migration did not
complete, we should print some human-readable descriptions.

This commit implements this. This logic may also be useful for the
devkit comment generation, or CLI usage.

In addition, we expose another VSCode refactoring to try via best effort
mode. There is no way for prompting, or adding multiple actions for the
same refactoring, so we expose a new refactoring.

PR Close #57659
This commit is contained in:
Paul Gschwendtner 2024-09-04 14:04:22 +00:00 committed by Jessica Janiuk
parent 9c31ba95e5
commit f694acb587
5 changed files with 163 additions and 110 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

85
packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.md
View file

@ -1,85 +0,0 @@
# Possible reasons why an `@Input` is not migrated
The signal input migration may skip migration of `@Input()` declarations if the automated refactoring isn't possible or safe.
This document explains some of the reasons, or better known "migration incompatibilities".
### `REASON:Accessor`
The input is declared using a getter/setter.
This is non-trivial to migrate without knowing the intent.
### `REASON:WriteAssignment`
Parts of your application write to this input class field.
This blocks the automated migration because signal inputs cannot be modified programmatically.
### `REASON:OverriddenByDerivedClass`
The input is part of a class that is extended by another class which overrides the field.
Migrating the input would then cause type conflicts and break the application build.
```ts
@Component()
class MyComp {
@Input() myInput = true;
}
class Derived extends MyComp {
override myInput = false; // <-- this wouldn't be a signal input and break!
}
```
### `REASON:RedeclaredViaDerivedClassInputsArray`
The input is part of a class that is extended by another class which overrides the field via the `inputs` array of `@Component` or `@Directive`.
Migrating the input would cause a mismatch because fields declared via `inputs` cannot be signal inputs.
```ts
@Component()
class MyComp {
@Input() myInput = true;
}
@Component({
inputs: ['myInput: aliasedName']
})
class Derived extends MyComp {}
```
### `REASON:TypeConflictWithBaseClass`
The input is overriding a field from a superclass, but the superclass field is not an Angular `@Input` and is not migrated.
This results in a type conflict and would break the build.
```ts
interface Parent {
disabled: boolean;
}
@Component()
class MyComp implements Parent {
@Input() disabled = true;
}
```
### `REASON:ParentIsIncompatible`
The input is overriding a field from a superclass, but the superclass field could not be migrated.
This means that migrating the input would break the build.
### `REASON:SpyOnThatOverwritesField`
The input can be migrated, but a Jasmine `spyOn` call for the input field was discovered.
`spyOn` calls are incompatible with signal inputs because they attempt to override the value of the field.
Signal inputs cannot be changed programmatically though— so this breaks.
### `REASON:PotentiallyNarrowedInTemplateButNoSupportYet`
The input is part of an `@if` or `*ngIf`, or template input in general.
This indicates that the input value type may be narrowed.
The migration skips migrating such inputs because support for narrowed signals is not available yet.
### `REASON:RequiredInputButNoGoodExplicitTypeExtractable`
The input is required, but cannot be safely migrated because no good type could be detected.
A required input with initial value doesn't make sense.
The type is inferred with `@Input` via the initial value, but this isn't possible with `input.required<T>`.
```ts
class MyComp {
@Input({required: true}) bla = someComplexInitialValue();
}
```

106
packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts Normal file
View file

@ -0,0 +1,106 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {ClassIncompatibilityReason, InputIncompatibilityReason} from './incompatibility';
/**
* Gets human-readable message information for the given input incompatibility.
* This text will be used by the language service, or CLI-based migration.
*/
export function getMessageForInputIncompatibility(reason: InputIncompatibilityReason): {
short: string;
extra: string;
} {
switch (reason) {
case InputIncompatibilityReason.Accessor:
return {
short: 'Accessor inputs cannot be migrated as they are too complex.',
extra:
'The migration potentially requires usage of `effect` or `computed`, but ' +
'the intent is unclear. The migration cannot safely migrate.',
};
case InputIncompatibilityReason.OverriddenByDerivedClass:
return {
short: 'The input cannot be migrated because the field is overridden by a subclass.',
extra: 'The field in the subclass is not an input, so migrating would break your build.',
};
case InputIncompatibilityReason.ParentIsIncompatible:
return {
short: 'This input is inherited from a superclass, but the parent cannot be migrated.',
extra: 'Migrating this input would cause your build to fail.',
};
case InputIncompatibilityReason.PotentiallyNarrowedInTemplateButNoSupportYet:
return {
short:
'This input is used in a control flow expression (e.g. `@if` or `*ngIf`) and ' +
'migrating would break narrowing currently.',
extra: `In the future, Angular intends to support narrowing of signals.`,
};
case InputIncompatibilityReason.RedeclaredViaDerivedClassInputsArray:
return {
short: 'The input is overridden by a subclass that cannot be migrated.',
extra:
'The subclass re-declares this input via the `inputs` array in @Directive/@Component. ' +
'Migrating this input would break your build because the subclass input cannot be migrated.',
};
case InputIncompatibilityReason.RequiredInputButNoGoodExplicitTypeExtractable:
return {
short: `Input is required, but the migration cannot determine a good type for the input.`,
extra: 'Consider adding an explicit type to make the migration possible.',
};
case InputIncompatibilityReason.SkippedViaConfigFilter:
return {
short: `This input is not part of the current migration scope.`,
extra: 'Skipped via migration config.',
};
case InputIncompatibilityReason.SpyOnThatOverwritesField:
return {
short: 'A jasmine `spyOn` call spies on this input. This breaks with signal inputs.',
extra: `Migration cannot safely migrate as "spyOn" writes to the input. Signal inputs are readonly.`,
};
case InputIncompatibilityReason.TypeConflictWithBaseClass:
return {
short:
'This input overrides a field from a superclass, while the superclass field is not migrated.',
extra: 'Migrating the input would break your build because of a type conflict then.',
};
case InputIncompatibilityReason.WriteAssignment:
return {
short: 'Your application code writes to the input. This prevents migration.',
extra: 'Signal inputs are readonly, so migrating would break your build.',
};
}
}
/**
* Gets human-readable message information for the given input class incompatibility.
* This text will be used by the language service, or CLI-based migration.
*/
export function getMessageForClassIncompatibility(reason: ClassIncompatibilityReason): {
short: string;
extra: string;
} {
switch (reason) {
case ClassIncompatibilityReason.InputOwningClassReferencedInClassProperty:
return {
short: 'Class of this input is referenced in the signature of another class.',
extra:
'The other class is likely typed to expect a non-migrated field, so ' +
'migration is skipped to not break your build.',
};
case ClassIncompatibilityReason.ClassManuallyInstantiated:
return {
short:
'Class of this input is manually instantiated (`new Cmp()`). ' +
'This is discouraged and prevents migration',
extra:
'Signal inputs require a DI injection context. Manually instantiating ' +
'breaks this requirement in some cases, so the migration is skipped.',
};
}
}

62
packages/language-service/src/refactorings/convert_to_signal_input.ts
View file

@ -11,29 +11,29 @@ import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
import {getFileSystem} from '@angular/compiler-cli/src/ngtsc/file_system';
import {MetaKind} from '@angular/compiler-cli/src/ngtsc/metadata';
import {
ClassIncompatibilityReason,
InputIncompatibilityReason,
} from '@angular/core/schematics/migrations/signal-migration/src/input_detection/incompatibility';
getMessageForClassIncompatibility,
getMessageForInputIncompatibility,
} from '@angular/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human';
import {ApplyRefactoringProgressFn} from '@angular/language-service/api';
import ts from 'typescript';
import {isInputContainerNode} from '../../../core/schematics/migrations/signal-migration/src/input_detection/input_node';
import {SignalInputMigration} from '../../../core/schematics/migrations/signal-migration/src/migration';
import {groupReplacementsByFile} from '../../../core/schematics/utils/tsurge/helpers/group_replacements';
import {isInputContainerNode} from '@angular/core/schematics/migrations/signal-migration/src/input_detection/input_node';
import {SignalInputMigration} from '@angular/core/schematics/migrations/signal-migration/src/migration';
import {MigrationConfig} from '@angular/core/schematics/migrations/signal-migration/src/migration_config';
import {groupReplacementsByFile} from '@angular/core/schematics/utils/tsurge/helpers/group_replacements';
import {isTypeScriptFile} from '../utils';
import {findTightestNode, getParentClassDeclaration} from '../utils/ts_utils';
import type {ActiveRefactoring} from './refactoring';
import {isTypeScriptFile} from '../utils';
/**
* Language service refactoring action that can convert `@Input()`
* Base language service refactoring action that can convert `@Input()`
* declarations to signal inputs.
*
* The user can click on an `@Input` property declaration in e.g. the VSCode
* extension and ask for the input to be migrated. All references, imports and
* the declaration are updated automatically.
*/
export class ConvertToSignalInputRefactoring implements ActiveRefactoring {
static id = 'convert-to-signal-input';
static description = '(experimental fixer): Convert @Input() to a signal input';
abstract class BaseConvertToSignalInputRefactoring implements ActiveRefactoring {
abstract config: MigrationConfig;
static isApplicable(
compiler: NgCompiler,
@ -106,6 +106,7 @@ export class ConvertToSignalInputRefactoring implements ActiveRefactoring {
const fs = getFileSystem();
const migration = new SignalInputMigration({
...this.config,
upgradeAnalysisPhaseToAvoidBatch: true,
reportProgressFn: reportProgress,
shouldMigrateInput: (input) => input.descriptor.node === containingProp,
@ -145,17 +146,29 @@ export class ConvertToSignalInputRefactoring implements ActiveRefactoring {
const {container, descriptor} = targetInput;
const memberIncompatibility = container.memberIncompatibility.get(descriptor.key);
const classIncompatibility = container.incompatible;
const aggressiveModeRecommendation = !this.config.bestEffortMode
? `\n—— Consider using the "(forcibly, ignoring errors)" action to forcibly convert.`
: '';
if (memberIncompatibility !== undefined) {
const {short, extra} = getMessageForInputIncompatibility(memberIncompatibility.reason);
return {
edits: [],
notApplicableReason: `${short}\n${extra}${aggressiveModeRecommendation}`,
};
}
if (classIncompatibility !== null) {
const {short, extra} = getMessageForClassIncompatibility(classIncompatibility);
return {
edits: [],
notApplicableReason: `${short}\n${extra}${aggressiveModeRecommendation}`,
};
}
return {
edits: [],
// TODO: Output a better human-readable message here. For now this is better than a noop.
notApplicableReason: `Input cannot be migrated: ${
memberIncompatibility !== undefined
? InputIncompatibilityReason[memberIncompatibility.reason]
: classIncompatibility !== null
? ClassIncompatibilityReason[classIncompatibility]
: 'unknown'
}`,
notApplicableReason:
'Input cannot be migrated, but no reason was found. ' +
'Consider reporting a bug to the Angular team.',
};
}
@ -184,6 +197,17 @@ export class ConvertToSignalInputRefactoring implements ActiveRefactoring {
}
}
export class ConvertToSignalInputRefactoring extends BaseConvertToSignalInputRefactoring {
static id = 'convert-to-signal-input-safe-mode';
static description = 'Convert this @Input() to a signal input (safe)';
override config: MigrationConfig = {};
}
export class ConvertToSignalInputBestEffortRefactoring extends BaseConvertToSignalInputRefactoring {
static id = 'convert-to-signal-input-best-effort-mode';
static description = 'Convert @Input() to a signal input (forcibly, ignoring errors)';
override config: MigrationConfig = {bestEffortMode: true};
}
function findParentPropertyDeclaration(node: ts.Node): ts.PropertyDeclaration | null {
while (!ts.isPropertyDeclaration(node) && !ts.isSourceFile(node)) {
node = node.parent;

10
packages/language-service/src/refactorings/refactoring.ts
View file

@ -10,7 +10,10 @@ import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core';
import ts from 'typescript';
import {ApplyRefactoringProgressFn} from '@angular/language-service/api';
import {CompilerOptions} from '@angular/compiler-cli';
import {ConvertToSignalInputRefactoring} from './convert_to_signal_input';
import {
ConvertToSignalInputBestEffortRefactoring,
ConvertToSignalInputRefactoring,
} from './convert_to_signal_input';
/**
* Interface exposing static metadata for a {@link Refactoring},
@ -60,4 +63,7 @@ export interface ActiveRefactoring {
): Promise<ts.RefactorEditInfo>;
}
export const allRefactorings: Refactoring[] = [ConvertToSignalInputRefactoring];
export const allRefactorings: Refactoring[] = [
ConvertToSignalInputRefactoring,
ConvertToSignalInputBestEffortRefactoring,
];

10
packages/language-service/test/signal_input_refactoring_action_spec.ts
View file

@ -34,8 +34,9 @@ describe('Signal input refactoring action', () => {
appFile.moveCursorToText('bl¦a');
const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor);
expect(refactorings.length).toBe(1);
expect(refactorings[0].name).toBe('convert-to-signal-input');
expect(refactorings.length).toBe(2);
expect(refactorings[0].name).toBe('convert-to-signal-input-safe-mode');
expect(refactorings[1].name).toBe('convert-to-signal-input-best-effort-mode');
});
it('should not support refactoring a non-Angular property', () => {
@ -95,8 +96,9 @@ describe('Signal input refactoring action', () => {
appFile.moveCursorToText('bl¦a');
const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor);
expect(refactorings.length).toBe(1);
expect(refactorings[0].name).toBe('convert-to-signal-input');
expect(refactorings.length).toBe(2);
expect(refactorings[0].name).toBe('convert-to-signal-input-safe-mode');
expect(refactorings[1].name).toBe('convert-to-signal-input-best-effort-mode');
const edits = await project.applyRefactoring(
'app.ts',