New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(compiler): correct confusion between field and property names #38685
Conversation
9bf544b
to
0f0bbea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring @alxhub!
/** | ||
* The field name on the component or directive instance for this input or output. | ||
*/ | ||
fieldName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fieldName: string; | |
fieldName: FieldName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
|
||
/** | ||
* An input or output of a directive that has both a JavaScript field name as well as an Angular | ||
* template property name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Good call.
type FieldName = string; | ||
type PropertyName = string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
0f0bbea
to
6923525
Compare
} | ||
|
||
/** | ||
* Lookup all `InputOrOutput`s that use this `propertyName`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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.
6923525
to
54b551c
Compare
…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.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The
R3TargetBinder
accepts an interface for directive metadata whichdeclares types for
input
andoutput
objects. These types convey themapping between the property names for an input or output and the
corresponding field name on the component class. Due to
R3TargetBinder
'srequirements, 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.