fix(compiler): correct confusion between field and property names #38685

alxhub · 2020-09-02T19:51:16Z

The R3TargetBinder accepts an interface for directive metadata which
declares types for input and output objects. These types convey the
mapping between the property names for an input or output and the
corresponding field name on the component class. Due to R3TargetBinder's
requirements, this mapping was specified with property names as keys and
field names as values.

However, because of duck typing, this interface was accidentally satisifed
by the opposite mapping, of field names to property names, that was produced
in other parts of the compiler. This form more naturally represents the data
model for inputs.

Rather than accept the field -> property mapping and invert it, this commit
introduces a new abstraction for field to property mappings which is
bidirectional, eliminating the ambiguous plain object type. This mapping can
be used to satisfy both the needs of the binder (property -> field) as well
as those of the template type-checker (field -> property).

A new test ensures that the input/output metadata produced by the compiler
during analysis is directly compatible with the binder via this unambiguous
new interface.

petebacondarwin

Nice refactoring @alxhub!

petebacondarwin · 2020-09-02T20:04:24Z

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

+  /**
+   * The field name on the component or directive instance for this input or output.
+   */
+  fieldName: string;


Suggested change

fieldName: string;

fieldName: FieldName;

The alias types FieldName and PropertyName are not exposed/exported - they're only used internally as documentation. It doesn't make sense to expose them as they're not branded types, so they provide no extra protection relative to string.

I guess for documentation's sake it might be reasonable, so I've gone ahead with that. It's not perfect though since TS doesn't allow a custom type as the key in index signatures, even a direct alias of string.

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

petebacondarwin · 2020-09-02T20:07:03Z

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

+
+/**
+ * An input or output of a directive that has both a JavaScript field name as well as an Angular
+ * template property name.


I find field and property still a bit ambiguous in my mind - I have to think hard to remember which is which. How about renaming these to something like: componentProperty and templateProperty?

Done! Good call.

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

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

JoostK · 2020-09-02T20:42:17Z

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

+type FieldName = string;
+type PropertyName = string;


Would it be beneficial to make these branded types and avoid incorrect assignments? Might take some effort if we want to apply that throughout the compiler code, so could be seen as a follow-up change.

Potentially, yeah. A tricky part of this is that @angular/compiler consumes these interfaces too, so we'd have to share the types somehow. I agree it'd make a good followup.

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

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

atscott

👍

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

packages/compiler/test/render3/view/binding_spec.ts

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

JoostK · 2020-09-03T18:38:48Z

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

+  }
+
+  /**
+   * Lookup all `InputOrOutput`s that use   this `propertyName`.


Suggested change

* Lookup all `InputOrOutput`s that use this `propertyName`.

* Lookup all `InputOrOutput`s that use this `propertyName`.

The `R3TargetBinder` accepts an interface for directive metadata which declares types for `input` and `output` objects. These types convey the mapping between the property names for an input or output and the corresponding property name on the component class. Due to `R3TargetBinder`'s requirements, this mapping was specified with property names as keys and field names as values. However, because of duck typing, this interface was accidentally satisifed by the opposite mapping, of field names to property names, that was produced in other parts of the compiler. This form more naturally represents the data model for inputs. Rather than accept the field -> property mapping and invert it, this commit introduces a new abstraction for such mappings which is bidirectional, eliminating the ambiguous plain object type. This mapping uses new, unambiguous terminology ("class property name" and "binding property name") and can be used to satisfy both the needs of the binder as well as those of the template type-checker (field -> property). A new test ensures that the input/output metadata produced by the compiler during analysis is directly compatible with the binder via this unambiguous new interface.

…8685) The `R3TargetBinder` accepts an interface for directive metadata which declares types for `input` and `output` objects. These types convey the mapping between the property names for an input or output and the corresponding property name on the component class. Due to `R3TargetBinder`'s requirements, this mapping was specified with property names as keys and field names as values. However, because of duck typing, this interface was accidentally satisifed by the opposite mapping, of field names to property names, that was produced in other parts of the compiler. This form more naturally represents the data model for inputs. Rather than accept the field -> property mapping and invert it, this commit introduces a new abstraction for such mappings which is bidirectional, eliminating the ambiguous plain object type. This mapping uses new, unambiguous terminology ("class property name" and "binding property name") and can be used to satisfy both the needs of the binder as well as those of the template type-checker (field -> property). A new test ensures that the input/output metadata produced by the compiler during analysis is directly compatible with the binder via this unambiguous new interface. PR Close #38685

angular#38685 corrected the confusion between field and property names so the consumer can now be determined correctly.

#38685 corrected the confusion between field and property names so the consumer can now be determined correctly. PR Close #38798

angular-automatic-lock-bot · 2020-10-09T16:25:59Z

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.}

googlebot added the cla: yes label Sep 2, 2020

petebacondarwin added area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release type: bug/fix labels Sep 2, 2020

ngbot bot modified the milestone: needsTriage Sep 2, 2020

alxhub force-pushed the ngtsc/fix/t2-binder-input-output branch from 9bf544b to 0f0bbea Compare September 2, 2020 20:04

petebacondarwin reviewed Sep 2, 2020

View reviewed changes

JoostK reviewed Sep 2, 2020

View reviewed changes

atscott reviewed Sep 2, 2020

View reviewed changes

alxhub force-pushed the ngtsc/fix/t2-binder-input-output branch from 0f0bbea to 6923525 Compare September 3, 2020 18:28

JoostK approved these changes Sep 3, 2020

View reviewed changes

atscott approved these changes Sep 3, 2020

View reviewed changes

alxhub force-pushed the ngtsc/fix/t2-binder-input-output branch from 6923525 to 54b551c Compare September 3, 2020 21:32

atscott added the action: merge The PR is ready for merge by the caretaker label Sep 8, 2020

atscott closed this in 4007422 Sep 8, 2020

atscott added a commit to atscott/angular that referenced this pull request Sep 10, 2020

test(compiler): Add back tests for renamed inputs and outputs

5979012

angular#38685 corrected the confusion between field and property names so the consumer can now be determined correctly.

atscott mentioned this pull request Sep 10, 2020

test(compiler): Add back tests for renamed inputs and outputs #38798

Closed

AndrewKushnir pushed a commit that referenced this pull request Sep 10, 2020

test(compiler): Add back tests for renamed inputs and outputs (#38798)

2d52c80

#38685 corrected the confusion between field and property names so the consumer can now be determined correctly. PR Close #38798

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): correct confusion between field and property names #38685

fix(compiler): correct confusion between field and property names #38685

alxhub commented Sep 2, 2020

petebacondarwin left a comment

petebacondarwin Sep 2, 2020

alxhub Sep 3, 2020

alxhub Sep 3, 2020

petebacondarwin Sep 2, 2020

alxhub Sep 3, 2020

JoostK Sep 2, 2020

alxhub Sep 3, 2020

atscott left a comment

JoostK Sep 3, 2020

angular-automatic-lock-bot bot commented Oct 9, 2020

	* Lookup all `InputOrOutput`s that use this `propertyName`.
	* Lookup all `InputOrOutput`s that use this `propertyName`.

fix(compiler): correct confusion between field and property names #38685

fix(compiler): correct confusion between field and property names #38685

Conversation

alxhub commented Sep 2, 2020

petebacondarwin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Oct 9, 2020