feat(ivy): input type coercion for template type-checking #33243

alxhub · 2019-10-17T23:15:14Z

Often the types of an @Input's field don't fully reflect the types of
assignable values. This can happen when an input has a getter/setter pair
where the getter always returns a narrow type, and the setter coerces a
wider value down to the narrow type.

For example, you could imagine an input of the form:

@Input() get value(): string {
  return this._value;
}

set value(v: {toString(): string}) {
  this._value = v.toString();
}

Here, the getter always returns a string, but the setter accepts any value
that can be toString()'d, and coerces it to a string.

Unfortunately TypeScript does not actually support this syntax, and so
Angular users are forced to type their setters as narrowly as the getters,
even though at runtime the coercion works just fine.

To support these kinds of patterns (e.g. as used by Material), this commit
adds a compiler feature called "input coercion". When a binding is made to
the 'value' input of a directive like MatInput, the compiler will look for a
static function with the name ngCoerceInput_value. If such a function is
found, the type-checking expression for the input will be wrapped in a call
to the function, allowing for the expression of a type conversion between
the binding expression and the value being written to the input's field.

To solve the case above, for example, MatInput might write:

class MatInput {
  // rest of the directive...

  static ngCoerceInput_value(value: {toString(): string}): string {
    return null!;
  }
}

FW-1475 #resolve

trotyl · 2019-10-18T01:54:24Z

packages/compiler-cli/src/ngtsc/metadata/src/util.ts

+  if (!member.name.startsWith('ngCoerceInput_')) {
+    return null !;
+  }
+  return member.name.split('_', 2)[1];


What if the @Input() property name itself contains _?

That's a good point. Fixed :)

alxhub · 2019-10-19T00:24:54Z

Presubmit

Airblader · 2019-10-19T12:48:30Z

Is there any compile-time error if the coercion function doesn't type-match the input (eg in your example if it is typed to return boolean)?

The fact that it requires a static function also is a bit awkward for component inheritance as static functions are not inherited. It also prevents using any non-static methods for the coercion (which a setter does have access to).

Overall this feels like a workaround to remove type safety in the template type checking. Material could just as well have pipes for common coercion ([value]="foo | stringify") or use the wider type for the input setter (and remove the getter) and expose the narrowed one as a separate property.

Airblader · 2019-10-19T13:06:26Z

I can't test this right now since I'm on mobile, but wouldn't the following also work? This might actually be an error, in which case disregard.

export class FooComponent {
    @Input("value") 
    set matCoerce_value(v: {toString(): string}) {
        this.value = v.toString();
    } 

    // and now either this or just a property if setter/getter isn't needed for anything else 
    set value(value: string) { ... } 
    get value(): string { ... } 
}

It binds the wider type as a setter for ease of use in templates but uses the narrower type for the actual property on the class of the same name as the input. Conceptually this is basically the same as the proposal but doesn't require new APIs or static functions.

Edit: This does seem to work. Is there any reason why this wouldn't be used?

JoostK · 2019-10-20T12:21:39Z

I noticed that the produced error message refers to arguments and parameters, which looks a bit weird from the template's perspective.

We could introduce some TypeScript magic to obtain the type of the coercion's input

class TestDir {
    value!: number;
    static ngCoerceInput_value: (param: string | number) => number;
}

type CoercedInput<Fn> = Fn extends (type: infer T) => any ? T : never;

// Before, without any coercion:
const _ctor: (p: Pick<TestDir, 'value'>) => TestDir = null!;

_ctor({
    value: '1',
});

_ctor({
    value: true,
});


// After, with coercion magic:
type TestDir_Bindings = Pick<TestDir, 'value'> | { value: CoercedInput<typeof TestDir.ngCoerceInput_value> };
const _ctorCoerce: (p: TestDir_Bindings) => TestDir = null!;

_ctorCoerce({
    value: '1',
});

_ctorCoerce({
    value: true,
});

Playground link

Note that the return type of ngCoerceInput_value is no longer used, so we could simplify things and simply declare ngCoerceInput_value: string | number;, completely eliminating the need for CoercedInput. This also avoids the need to keep the return type of the coercion function in sync with the actually declared type.

I am also interested in @Airblader's approach, which does not require additional magic from Angular's compiler side and is more strict wrt the directive's implementation, at the expense of an awkward syntax AFAIK. If, however, TypeScript starts to allow setters with a wider type than the getter this issue may eventually resolve itself.

Airblader · 2019-10-20T12:26:36Z

at the expense of an awkward syntax AFAIK

I won't argue it's a bit awkward, but I don't think it's more awkward than the proposal in the PR. :-)

trotyl · 2019-10-20T12:52:46Z

The fact that it requires a static function also is a bit awkward for component inheritance as static functions are not inherited.

Actually static class methods are indeed inherited in JavaScript.

Airblader · 2019-10-20T13:42:00Z

You're right; static functions are used so rarely I misremembered that. My other points still stand, though.

If this PR should move forward either way I would question why it has to be a static method. Logically speaking there's no reason, I assume it's technical due to the ahead of time compilation?

rbuzzibarrios · 2019-10-20T23:44:10Z

packages/compiler-cli/src/ngtsc/metadata/src/util.ts

+  if (!member.name.startsWith('ngCoerceInput_')) {
+    return null !;
+  }
+  return afterUnderscore(member.name);


blank line before return ?

IgorMinar · 2019-10-21T15:56:05Z

@Airblader this got merged by accident - we haven't finished reviewing the PR. I asked @matsko to revert and reopen it. Sorry about sending wrong signals.

…gular#33243) This reverts commit 1b4eaea.

…3243) (#33299) This reverts commit 1b4eaea. PR Close #33299

matsko · 2019-10-21T16:06:07Z

This was pre-maturely merged. My mistake. This is open again now.

IgorMinar · 2019-10-22T01:41:24Z

packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

+        export declare class MatInput {
+          value: string;
+          static ɵdir: i0.ɵɵDirectiveDefWithMeta<MatInput, '[matInput]', never, {'value': 'value'}, {}, never>;
+          static ngAcceptInputType_value: string|number;


how about ngInputTypeOverride?

or even more intuitive would be if we passed in the type info via the @Input decorator:

@Input<string|null>() set value(value: string) { ... } get value(): string { ... }

Considering that there is a number of options to pick from and this being a feature that we don't strictly need for v9, I think we should defer this until 9.1

@IgorMinar the problem with this approach is that the decorator gets removed during compilation, so nothing carries the type forward into the .d.ts file. So the compiler would have to add it in this case.

For performance, we need the field to actually exist in the .d.ts file when a library ships.

I'm happy to defer this, but know that this effectively blocks using strictInputTypes for Material. We should probably disable that option by default, then, so users don't trip over fullTemplateTypeCheck giving unfixable error messages for Material.

I asked @jelbourn to weigh in on the current PR and impact of deferring this to 9.1. Let's get his input and reevaluate what to do.

@jelbourn tested the PR and gave 👍

Jeremy also argued that ngAcceptInputType_ is a more descriptive name than other suggestions.

IgorMinar

#33243 (comment)

@input

) Often the types of an `@Input`'s field don't fully reflect the types of assignable values. This can happen when an input has a getter/setter pair where the getter always returns a narrow type, and the setter coerces a wider value down to the narrow type. For example, you could imagine an input of the form: ```typescript @input() get value(): string { return this._value; } set value(v: {toString(): string}) { this._value = v.toString(); } ``` Here, the getter always returns a `string`, but the setter accepts any value that can be `toString()`'d, and coerces it to a string. Unfortunately TypeScript does not actually support this syntax, and so Angular users are forced to type their setters as narrowly as the getters, even though at runtime the coercion works just fine. To support these kinds of patterns (e.g. as used by Material), this commit adds a compiler feature called "input coercion". When a binding is made to the 'value' input of a directive like MatInput, the compiler will look for a static function with the name ngCoerceInput_value. If such a function is found, the type-checking expression for the input will be wrapped in a call to the function, allowing for the expression of a type conversion between the binding expression and the value being written to the input's field. To solve the case above, for example, MatInput might write: ```typescript class MatInput { // rest of the directive... static ngCoerceInput_value(value: {toString(): string}): string { return null!; } } ``` FW-1475 #resolve PR Close angular#33243

…gular#33243) (angular#33299) This reverts commit 1b4eaea. PR Close angular#33299

IgorMinar · 2019-10-23T01:53:25Z

@Airblader your proposed would require breaking changes for all component libraries that would want to be compatible with strict template type-checking. The approach we are looking for is something that can be incrementally added to component libraries and would enable strict type checking of all the inputs, while still supporting the common cases of input type coercing.

One point you bring up that should however be addressed is that the type used for type checking the setter should be a super set of the type specified on the getter. Otherwise things we could get into bizarre situation where the setter claims to accept e.g. string | boolean and the getter would declare it's type to be number. I'm not aware of use-cases when this would be desirable. @alxhub can you comment on this? thanks

Airblader · 2019-10-23T06:37:14Z

Thanks, I appreciate it. I have a couple more points / questions:

I assume this will cause IDEs to report these fields as unused until IDEs "learn" to parse this Angular specific feature, right?
Is it required for this static field to be public? Can it be private? If so, I think examples should prefer private (to not leak this) and either way it should be part of the documentation.

IgorMinar · 2019-10-23T13:20:33Z

@Airblader IDEs would report these fields as used/unused in the same way the report component class properties that are used only in templates.

The field doesn't have be public (private fields are present in d.ts files), but this is part of the public api, so making them private would hide important public api info.

Airblader · 2019-10-23T13:53:02Z

but this is part of the public api, so making them private would hide important public api info.

Well, it's part of a public API: the template-facing one. The following if I'm not mistaken:

@Component({…})
export class FooComponent {
    static ngAcceptInputType_width: number | string;

    @Input()
    public set width(w: number) {
        this._width = parseInt(w as any, 10);
    }

    private _width: number;
}

@Component({…})
export class BarComponent {
    @ViewChild(FooComponent) foo;

    ngAfterViewInit() {
        this.foo.width = "42"; // error
    }
}

… will continue not to work, right? ngAcceptInputType_ is really just a way to trick the TS compiler and (partially) break the very type safety of strict template type checking we want to enable. ;-)

In the actual input setter since the more narrow type is used, TS will not be able to type-check that a conversion of the wider type to the narrower type actually ever happens, i.e. the following will compile just fine and cause the very bugs TS is designed to prevent:

@Component({…})
export class FooComponent {
    static ngAcceptInputType_width: number | string;

    @Input()
    public set width(w: number) {
        // What really happens is that we assign a string to a number, but TS is unable to tell us
        this._width = w;
    }

    private _width: number;
}

Admittedly this is about the same situation as we have without strict template type checking. In any case, I have said my peace and do not want to cause too much trouble going forward with this.

alxhub · 2019-10-23T20:57:50Z

@Airblader your input is greatly appreciated!

Responses to your points:

I assume this will cause IDEs to report these fields as unused until IDEs "learn" to parse this Angular specific feature, right?

No, because the field is not a private instance field, it's visible to consumers and so is never considered unused. There is a risk that a future version of TypeScript could be stricter about requiring such fields to have values, but that can be easily migrated if needed.

Is it required for this static field to be public? Can it be private? If so, I think examples should prefer private (to not leak this) and either way it should be part of the documentation.

I actually had no idea private static was a thing. That's cool!

Unfortunately no, like with @Input fields, this is part of the public API of the class and thus needs to be public. The type-checker won't be able to generate writes to the field otherwise.

Airblader · 2019-10-23T20:59:16Z

your proposed would require breaking changes for all component libraries that would want to be compatible with strict template type-checking.

Maybe a moot point now, but upon reconsidering I realized I don't understand where the breaking change would lie in that approach. In fact the API would look the same coming from either a template or programmatic access. The only difference (besides allowing the wider type in templates) is that there's an additional setter, but that's not breaking and also not much different from the change in this PR. Perhaps I'm missing something, though.

alxhub · 2019-10-23T21:08:30Z

Maybe a moot point now, but upon reconsidering I realized I don't understand where the breaking change would lie in that approach. In fact the API would look the same coming from either a template or programmatic access. The only difference (besides allowing the wider type in templates) is that there's an additional setter, but that's not breaking and also not much different from the change in this PR. Perhaps I'm missing something, though.

I don't think it's a breaking change in terms of the semantics today (though arguably, renaming an @Input is kinda iffy in the general case). I think it's problematic for a few reasons.

It's a much more painful change for libraries to go through, and not an obvious one that they need to make.
Getter/setter fields are expensive, particularly in ES5 code where they're downleveled to Object.defineProperty calls. The ngAcceptInputType_ solution has no runtime cost. For libraries like Material, duplicating the fields would mean a measurable code size increase.

To your other point:

ngAcceptInputType_ is really just a way to trick the TS compiler and (partially) break the very type safety of strict template type checking we want to enable. ;-)

This is definitely true. To me, it's like any other type assertion - it can be used to improve the typings if applied properly, or to subvert them if applied improperly. I added a line about this at the end of the new docs for this feature.

alxhub · 2019-10-23T21:13:42Z

@IgorMinar

Otherwise things we could get into bizarre situation where the setter claims to accept e.g. string | boolean and the getter would declare it's type to be number. I'm not aware of use-cases when this would be desirable. @alxhub can you comment on this? thanks

I agree this would be bizarre, and I can't think of a good reason for it.

But nothing precludes you from doing something like that in JS:

_value: number;

get value(): number /* number */ {
  return this._value;
}

set value(in: number /* string */) {
  this._value = parseInt(in as any as string);
}

static ngAcceptInputType_value: string;

Theoretically, the above code would be typechecked properly.

alxhub · 2019-10-23T21:38:26Z

Caretaker: I am the approver.

alxhub · 2019-10-23T21:42:33Z

Presubmit
No need for an Ivy presubmit, this does not affect g3 code as nobody is using the new type (or typechecker)

alxhub · 2019-10-23T22:11:41Z

Presubmit for real this time

ngbot · 2019-10-23T22:11:55Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: lint" is failing
     status "ci/angular: size" is pending
     status "ci/circleci: aio_preview" is pending
     status "ci/circleci: build-ivy-npm-packages" is pending
     status "ci/circleci: build-npm-packages" is pending
     status "ci/circleci: legacy-unit-tests-saucelabs" is pending
     status "ci/circleci: test" is pending
     status "ci/circleci: test_aio" is pending
     status "ci/circleci: test_ivy_aot" is pending
     status "google3" is pending
     missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@input

Often the types of an `@Input`'s field don't fully reflect the types of assignable values. This can happen when an input has a getter/setter pair where the getter always returns a narrow type, and the setter coerces a wider value down to the narrow type. For example, you could imagine an input of the form: ```typescript @input() get value(): string { return this._value; } set value(v: {toString(): string}) { this._value = v.toString(); } ``` Here, the getter always returns a `string`, but the setter accepts any value that can be `toString()`'d, and coerces it to a string. Unfortunately TypeScript does not actually support this syntax, and so Angular users are forced to type their setters as narrowly as the getters, even though at runtime the coercion works just fine. To support these kinds of patterns (e.g. as used by Material), this commit adds a compiler feature called "input coercion". When a binding is made to the 'value' input of a directive like MatInput, the compiler will look for a static field with the name ngAcceptInputType_value. If such a field is found the type-checking expression for the input will use the static field's type instead of the type for the @input field,allowing for the expression of a type conversion between the binding expression and the value being written to the input's field. To solve the case above, for example, MatInput might write: ```typescript class MatInput { // rest of the directive... static ngAcceptInputType_value: {toString(): string}; } ``` FW-1475 #resolve

IgorMinar · 2019-10-24T18:31:19Z

I just want to make one more note related to this feature. The primary reason for it is not to allow people to bypass typechecking everywhere, but to enable existing libraries that do accept wider type than the one defined via the getter to fully typecheck with strict template type checking. So essentially the code that works today, should keep on working and typecheck.

It is up to the library authors to decide if they want to take advantage of type coersion for inputs, considering that without the extra type information, they new code will fail to compile making it obvious that the setter type does not match the type used at runtime.

angular-automatic-lock-bot · 2019-11-24T16:10:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

alxhub requested a review from a team as a code owner October 17, 2019 23:15

googlebot added the cla: yes label Oct 17, 2019

alxhub force-pushed the ngtsc/ttc/input-coercion branch from cb90829 to fea1589 Compare October 17, 2019 23:24

trotyl reviewed Oct 18, 2019

View reviewed changes

alxhub force-pushed the ngtsc/ttc/input-coercion branch from fea1589 to 4af30d2 Compare October 18, 2019 17:41

alxhub added the target: major This PR is targeted for the next major release label Oct 18, 2019

alxhub force-pushed the ngtsc/ttc/input-coercion branch from 4af30d2 to e709743 Compare October 18, 2019 21:49

alxhub added the action: merge The PR is ready for merge by the caretaker label Oct 19, 2019

rbuzzibarrios reviewed Oct 20, 2019

View reviewed changes

matsko closed this in 1b4eaea Oct 21, 2019

This comment has been minimized.

Sign in to view

matsko added a commit to matsko/angular that referenced this pull request Oct 21, 2019

revert: feat(ivy): input type coercion for template type-checking (an…

5b7f428

…gular#33243) This reverts commit 1b4eaea.

matsko added a commit that referenced this pull request Oct 21, 2019

revert: feat(ivy): input type coercion for template type-checking (#3…

c0ebecf

…3243) (#33299) This reverts commit 1b4eaea. PR Close #33299

matsko reopened this Oct 21, 2019

alxhub force-pushed the ngtsc/ttc/input-coercion branch from e709743 to ae50c91 Compare October 22, 2019 00:02

IgorMinar reviewed Oct 22, 2019

View reviewed changes

IgorMinar suggested changes Oct 22, 2019

View reviewed changes

IgorMinar requested a review from jelbourn October 22, 2019 17:18

matsko added the comp: ivy label Oct 22, 2019

ngbot bot added this to the needsTriage milestone Oct 22, 2019

kara added the action: discuss label Oct 22, 2019

AndrusGerman pushed a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019

revert: feat(ivy): input type coercion for template type-checking (an…

2f4b612

…gular#33243) (angular#33299) This reverts commit 1b4eaea. PR Close angular#33299

IgorMinar force-pushed the ngtsc/ttc/input-coercion branch from ae50c91 to 678b2f0 Compare October 23, 2019 00:12

alxhub force-pushed the ngtsc/ttc/input-coercion branch from 678b2f0 to 69d11fa Compare October 23, 2019 21:16

alxhub added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 23, 2019

alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Oct 23, 2019

alxhub force-pushed the ngtsc/ttc/input-coercion branch from 69d11fa to 87bc229 Compare October 23, 2019 22:05

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 23, 2019

alxhub removed the action: discuss label Oct 23, 2019

alxhub force-pushed the ngtsc/ttc/input-coercion branch from 87bc229 to 46b5112 Compare October 23, 2019 22:12

AndrewKushnir closed this in f1269d9 Oct 24, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ivy): input type coercion for template type-checking #33243

feat(ivy): input type coercion for template type-checking #33243

alxhub commented Oct 17, 2019

trotyl Oct 18, 2019

alxhub Oct 18, 2019

alxhub commented Oct 19, 2019

Airblader commented Oct 19, 2019 •

edited

Airblader commented Oct 19, 2019 •

edited

JoostK commented Oct 20, 2019 •

edited

Airblader commented Oct 20, 2019

trotyl commented Oct 20, 2019

Airblader commented Oct 20, 2019

rbuzzibarrios Oct 20, 2019

This comment has been minimized.

IgorMinar commented Oct 21, 2019

matsko commented Oct 21, 2019

IgorMinar Oct 22, 2019

IgorMinar Oct 22, 2019

alxhub Oct 22, 2019

IgorMinar Oct 22, 2019

IgorMinar Oct 22, 2019

IgorMinar Oct 22, 2019

IgorMinar left a comment

IgorMinar commented Oct 23, 2019

Airblader commented Oct 23, 2019

IgorMinar commented Oct 23, 2019

Airblader commented Oct 23, 2019 •

edited

alxhub commented Oct 23, 2019

Airblader commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019 •

edited

alxhub commented Oct 23, 2019

ngbot bot commented Oct 23, 2019

IgorMinar commented Oct 24, 2019 •

edited

angular-automatic-lock-bot bot commented Nov 24, 2019

feat(ivy): input type coercion for template type-checking #33243

feat(ivy): input type coercion for template type-checking #33243

Conversation

alxhub commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub commented Oct 19, 2019

Airblader commented Oct 19, 2019 • edited

Airblader commented Oct 19, 2019 • edited

JoostK commented Oct 20, 2019 • edited

Airblader commented Oct 20, 2019

trotyl commented Oct 20, 2019

Airblader commented Oct 20, 2019

Choose a reason for hiding this comment

This comment has been minimized.

IgorMinar commented Oct 21, 2019

matsko commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar commented Oct 23, 2019

Airblader commented Oct 23, 2019

IgorMinar commented Oct 23, 2019

Airblader commented Oct 23, 2019 • edited

alxhub commented Oct 23, 2019

Airblader commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019

alxhub commented Oct 23, 2019 • edited

alxhub commented Oct 23, 2019

ngbot bot commented Oct 23, 2019

IgorMinar commented Oct 24, 2019 • edited

angular-automatic-lock-bot bot commented Nov 24, 2019

Airblader commented Oct 19, 2019 •

edited

Airblader commented Oct 19, 2019 •

edited

JoostK commented Oct 20, 2019 •

edited

Airblader commented Oct 23, 2019 •

edited

alxhub commented Oct 23, 2019 •

edited

IgorMinar commented Oct 24, 2019 •

edited