Skip to content
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

Set inputs for property #33798

Conversation

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Nov 13, 2019

Best to review commit-by-commit (individual commits have more details)

@googlebot googlebot added the cla: yes label Nov 13, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 13, 2019
@pkozlowski-opensource pkozlowski-opensource marked this pull request as ready for review Nov 13, 2019
@pkozlowski-opensource pkozlowski-opensource requested review from angular/fw-core as code owners Nov 13, 2019

class TestDirective {
// @Input() foo;
foo?: number;

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 13, 2019

Member

should set to default, so that the VM does not have to patch extra properties after constructor.

// @Input() foo;
foo?: number;
// @Input('bar') barPrivate;
barPrivate?: number;

This comment has been minimized.

Copy link
@mhevery
@@ -795,8 +795,8 @@ export function createTNode(
attrs, // attrs: (string|AttributeMarker|(string|SelectorFlags)[])[]|null
null, // localNames: (string|number)[]|null
undefined, // initialInputs: (string[]|null)[]|null|undefined
undefined, // inputs: PropertyAliases|null|undefined
undefined, // outputs: PropertyAliases|null|undefined
null, // inputs: PropertyAliases|null|undefined

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 13, 2019

Member

update comment (remove undefined) (next line also)

@kara
kara approved these changes Nov 13, 2019
Copy link
Contributor

kara left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 13, 2019

Before this change a public name of a directive's input
was stored in 2 places:
- as a key of an object on TNode.index;
- as a value of PropertyAliasValue at the index 1

This PR changes the data structure so the public name is stored
only once as a key on TNode.index. This saves one array entry
for each and every directive input.
TNode.inputs are initialised during directives resolution now so we know early
if a node has directives with inputs or no. We don't need to use undefined value
as an indicator that inputs were not resolved yet.
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member Author

pkozlowski-opensource commented Nov 18, 2019

merge-assistance due to " Status "google3" is pending"

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 18, 2019

Presubmit

I wanted to run a second one because the first one shows a failure as being definitely caused by this PR, for something which could not have failed as a result of this PR. So this is a sanity check.

@alxhub alxhub closed this in 5aec179 Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
Before this change a public name of a directive's input
was stored in 2 places:
- as a key of an object on TNode.index;
- as a value of PropertyAliasValue at the index 1

This PR changes the data structure so the public name is stored
only once as a key on TNode.index. This saves one array entry
for each and every directive input.

PR Close #33798
alxhub added a commit that referenced this pull request Nov 19, 2019
TNode.inputs are initialised during directives resolution now so we know early
if a node has directives with inputs or no. We don't need to use undefined value
as an indicator that inputs were not resolved yet.

PR Close #33798
alxhub added a commit that referenced this pull request Nov 19, 2019
Before this change a public name of a directive's input
was stored in 2 places:
- as a key of an object on TNode.index;
- as a value of PropertyAliasValue at the index 1

This PR changes the data structure so the public name is stored
only once as a key on TNode.index. This saves one array entry
for each and every directive input.

PR Close #33798
alxhub added a commit that referenced this pull request Nov 19, 2019
TNode.inputs are initialised during directives resolution now so we know early
if a node has directives with inputs or no. We don't need to use undefined value
as an indicator that inputs were not resolved yet.

PR Close #33798
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.