Skip to content

Commit

Permalink
fix(core): do not accidentally inherit input transforms when overridd…
Browse files Browse the repository at this point in the history
…en (angular#53571)

Currently when a base class defines an input with a transform, derived
classes re-defining the input via `@Input`, or `inputs: [<..>]`, end up
inherting the transform due to a bug in the inherit definitions feature.

This commit fixes this. We verified in the Google codebase that this is
an unlikely occurrence and it's trivial to fix on user side by removing
the re-declaration/override, or explictly adding the necessary
transform.

Conceptually, the behavior was quite inconsistent as everything else of
inputs was overridden as expected. i.e. alias, required state etc. The
exception were input transforms. This commit fixes this.

PR Close angular#53571
  • Loading branch information
devversion authored and amilamen committed Jan 26, 2024
1 parent ba42fe4 commit cb14bb2
Show file tree
Hide file tree
Showing 5 changed files with 222 additions and 152 deletions.
42 changes: 33 additions & 9 deletions packages/core/src/render3/features/inherit_definition_feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,9 @@ export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>|Compo
superContentQueries && inheritContentQueries(definition, superContentQueries);

// Merge inputs and outputs
fillProperties(definition.inputs, superDef.inputs);
fillProperties(definition.declaredInputs, superDef.declaredInputs);
mergeInputsWithTransforms(definition, superDef);
fillProperties(definition.outputs, superDef.outputs);

if (superDef.inputTransforms !== null) {
if (writeableDef.inputTransforms === null) {
writeableDef.inputTransforms = {};
}
fillProperties(writeableDef.inputTransforms, superDef.inputTransforms);
}

// Merge animations metadata.
// If `superDef` is a Component, the `data` field is present (defaults to an empty object).
if (isComponentDef(superDef) && superDef.data.animation) {
Expand Down Expand Up @@ -122,6 +114,38 @@ export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>|Compo
mergeHostAttrsAcrossInheritance(inheritanceChain);
}

function mergeInputsWithTransforms<T>(target: WritableDef, source: DirectiveDef<any>) {
for (const key in source.inputs) {
if (!source.inputs.hasOwnProperty(key)) {
continue;
}
if (target.inputs.hasOwnProperty(key)) {
continue;
}
const value = source.inputs[key];
if (value === undefined) {
continue;
}

target.inputs[key] = value;
target.declaredInputs[key] = source.declaredInputs[key];

// If the input is inherited, and we have a transform for it, we also inherit it.
// Note that transforms should not be inherited if the input has its own metadata
// in the `source` directive itself already (i.e. the input is re-declared/overridden).
if (source.inputTransforms !== null) {
// Note: transforms are stored with their minified names.
// Perf: only access the minified name when there are source transforms.
const minifiedName = Array.isArray(value) ? value[0] : value;
if (!source.inputTransforms.hasOwnProperty(minifiedName)) {
continue;
}
target.inputTransforms ??= {};
target.inputTransforms[minifiedName] = source.inputTransforms[minifiedName];
}
}
}

/**
* Merge the `hostAttrs` and `hostVars` from the inherited parent to the base class.
*
Expand Down
61 changes: 61 additions & 0 deletions packages/core/test/acceptance/inherit_definition_feature_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,67 @@ describe('inheritance', () => {
expect(subDir.baz).toBe('c');
expect(subDir.qux).toBe('d');
});

it('should not inherit transforms if inputs are re-declared', () => {
@Directive()
class Base {
@Input({transform: v => `${v}-transformed`}) someInput: string = '';
}

@Directive({
standalone: true,
selector: 'dir',
inputs: ['someInput'],
})
class ActualDir extends Base {
}

@Component({
standalone: true,
imports: [ActualDir],
template: `<dir someInput="newValue">`,
})
class TestCmp {
}

const fixture = TestBed.createComponent(TestCmp);
fixture.detectChanges();

const actualDir =
fixture.debugElement.query(By.directive(ActualDir)).injector.get(ActualDir);

expect(actualDir.someInput).toEqual('newValue');
});

it('should inherit transforms if inputs are aliased', () => {
@Directive()
class Base {
@Input({transform: v => `${v}-transformed`, alias: 'publicName'}) fieldName: string = '';
}

@Directive({
standalone: true,
selector: 'dir',
})
class ActualDir extends Base {
}

@Component({
standalone: true,
imports: [ActualDir],
template: `<dir publicName="newValue">`,
})
class TestCmp {
}

const fixture = TestBed.createComponent(TestCmp);
fixture.detectChanges();

const actualDir =
fixture.debugElement.query(By.directive(ActualDir)).injector.get(ActualDir);

expect(actualDir.fieldName).toEqual('newValue-transformed');
});
});

describe('outputs', () => {
Expand Down

0 comments on commit cb14bb2

Please sign in to comment.