fix(ivy): ngOnChanges to receive SimpleChanges with private non minif…

[marclaval]

…ied property names as keys

If my understanding is correct, with this code:

@Component({selector: 'lifecycle-comp', template: ``})
    class LifecycleComp {
      @Input('publicName') privateName: string;
      ngOnChanges(changes: SimpleChanges) {
         console.log(Object.keys(changes).join(',');
     }
   ...
}

Then, upon changes, 'privateName' will be logged in the console, not 'publicName' and not the minified version of 'privateName'.
If this behaviour is to be kept, we'd need to generate some data structure to keep this information. This PR is implementing a quite naive solution for that.

On top of that, it adds tests for the OnChange hook.

[vicb]

Not a "list" (may be fix the other API docs)

[vicb]

IMO we should drop "private" everywhere, it sounds misleading.

[vicb]

What about this terminology:

• alias = the name given in the decorator,
• property name = source code,
• minified name

[kara]

Agree that private is confusing. Interesting that I came to a similar conclusion but with alias flipped. e.g. public name is external facing, alias is internal-facing (if different).

[vicb]

My reasoning is that there is the thing (the property in our case) and you can give it any alias with @Input(alias) whatever the alias we always refer to the thing.

[kara]

Looks good to me aside from the name, but @alxhub or @mhevery should take a look too (in case they had a different plan for this)

[kara]

Looks like copy-pasted comment? Update to read onChanges?

[kara]

I think we could improve upon this name, esp since the property names of this will also be minified, which could be confusing. How about inputAliases?

[kara]

I think this test might be clearer if you show which parts would be minified. e.g.

inputs: {a: 'val1', b: 'publicName'},
inputsNonMinified: {b: 'val2'}

Same on line 1755

[kara]

Mention that these are private names or aliases to differentiate from inputs (same comment on line 40)?

[kara]

Same here, update to read onChanges?

[vicb]

may be rename inputToAlias, inputToProperty, outputToAlias if you think the terminology proposed below makes sense ?

[kara]

I think that's backwards for inputs and outputs, no? The public names end up as the keys after inverting, so wouldn't it be aliasToInput and aliasToOutput?

[vicb]

const * 3

[vicb]

What about being more explicit by changing
${name}${this.val1}${this.val2} - ${Object.keys(simpleChanges).join(',')}
for
comp=${name} val1=${this.val1} val2=${this.val2} - changed=[${Object.getOwnPropertyNames(simpleChanges).join(',')] }

[vicb]

"(and not in update mode)" ?

[vicb]

Added some comments.

Can't wait to have the JIT compiler so that we can write smaller integration tests without having to compile (and review) by hand !

[marclaval]

Thanks for your reviews, I've integrated your comments.

About the naming discussion, I've changed inputsNonMinified to inputsPropertyName as this dictionary is in fact not inverted as others. Still to be discussed ...

[vicb]

remove public

[vicb]

A dict is from something to something

[vicb]

Ok for me after my comments have been addressed.

Could you please open an issue for the alias naming ? Thanks.

[kara]

Looks good to me, apart from the naming. @marclaval created an issue to discuss naming with @mhevery returns here: #22383

[ngbot]

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/circleci: build" is failing
     missing required status "code-review/pullapprove"
     status "google3" is pending
     status "continuous-integration/travis-ci/pr" is pending
     2 pending code reviews

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.

[trotyl]

@IgorMinar FYI, current NgOnChangesFeature implementation would not work with the Stage 3 Class fields proposal due to the semantic change, which defines the property on the instance and will not trigger prototype setter, I guess you may also want to argue for it in the framework authors meeting.

_{But it's still possible to make it work by wrapping the factory method instead and hook on instances.}

[angular-automatic-lock-bot]

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