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

fix(common): reflect input type in context #33997

Closed
wants to merge 2 commits into from

Conversation

@crisbeto
Copy link
Member

crisbeto commented Nov 22, 2019

Fixes NgForOf not reflecting the type of its input in the NgForOfContext. Also fixes NgIf which was typed to any.

Fixes #33527.
Fixes #31556.

@googlebot googlebot added the cla: yes label Nov 22, 2019
@crisbeto crisbeto force-pushed the crisbeto:FW-1738/ngfor-input-type branch 2 times, most recently from 4d4e404 to adf9934 Nov 22, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 22, 2019
@crisbeto crisbeto marked this pull request as ready for review Nov 22, 2019
@crisbeto crisbeto requested review from angular/fw-core as code owners Nov 22, 2019
@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 26, 2019

Copy link
Member

IgorMinar left a comment

can you please split this into two commits and document each change and what is the user-facing fix/impact because when users read about this change in the changelog they will not have any idea about what this does, or mean to them.

I'm assuming that this is a fix for #33527. Can you please state that in the commit message?

Lastly, any chance we we can add some tests for this? I'm assuming that we currently don't have any good infra to test typechecking changes like this unless we write a ngtsc specific test. Is that right?

I've kicked off a presubmit for this change in the meantime to make sure that there are no surprises.

@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 26, 2019

also, this should target the 9.0.x branch as well because otherwise we won't be able to roll this out until v10. (I updated the target label)

Copy link
Member

JoostK left a comment

Regarding tests, I think the most valuable way would still be ngtsc's template type checking tests. I feel this way because those tests exercise real Angular templates.

As an alternative, a test could mimic the code that the type checker generates, as to assert that type inference works as expected. Such tests however are most appropriate to be run using https://github.com/microsoft/dtslint, which we don't currently have anywhere AFAIK. Also, I would consider them brittle as there's no guarantee that they are accurate, hence my preference for actual ngtsc template type checking tests.

packages/common/src/directives/ng_if.ts Show resolved Hide resolved
packages/common/src/directives/ng_if.ts Outdated Show resolved Hide resolved
@crisbeto crisbeto force-pushed the crisbeto:FW-1738/ngfor-input-type branch from adf9934 to 42c316d Nov 26, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Nov 26, 2019

I've addressed the feedback. Can you take another look @JoostK @IgorMinar?

@crisbeto crisbeto force-pushed the crisbeto:FW-1738/ngfor-input-type branch from 42c316d to df1e844 Nov 26, 2019
Copy link
Member

JoostK left a comment

I would love to see template type checker tests. If you need help with that, please let me know.

private _thenTemplateRef: TemplateRef<NgIfContext<T>> = null !;
private _elseTemplateRef: TemplateRef<NgIfContext<T>> = null !;
private _thenViewRef: EmbeddedViewRef<NgIfContext<T>> = null !;
private _elseViewRef: EmbeddedViewRef<NgIfContext<T>> = null !;

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 27, 2019

Member

All these private fields can keep the | null, as it keeps the implementation safe against nulls.

Copy link
Member

IgorMinar left a comment

fyi: g3 is looking good. @crisbeto can you please address Joost's feedback?

@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Nov 27, 2019

I have all the feedback addressed locally, but it uncovered some issues with the new signatures. I'll pair up with Joost tomorrow.

Fixes `NgForOf` not reflecting the type of its input in the `NgForOfContext`.
@crisbeto crisbeto force-pushed the crisbeto:FW-1738/ngfor-input-type branch from df1e844 to ac43930 Nov 28, 2019
@crisbeto crisbeto requested a review from angular/fw-compiler as a code owner Nov 28, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Nov 28, 2019

All the issues and feedback should addressed now. Thank you for the help with the tests @JoostK!

Copy link
Member

JoostK left a comment

In the NgIf commit, could you include a fixes note for #31556?

packages/common/src/directives/ng_if.ts Outdated Show resolved Hide resolved
Fixes the content of `NgIf` being typed to any.

Fixes #31556.
@crisbeto crisbeto force-pushed the crisbeto:FW-1738/ngfor-input-type branch from ac43930 to 66e8ed4 Nov 29, 2019
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Nov 29, 2019

Done.

@JoostK JoostK dismissed their stale review Dec 2, 2019

addressed

@alxhub
alxhub approved these changes Dec 2, 2019
mhevery added a commit that referenced this pull request Dec 2, 2019
Fixes `NgForOf` not reflecting the type of its input in the `NgForOfContext`.

PR Close #33997
mhevery added a commit that referenced this pull request Dec 2, 2019
Fixes the content of `NgIf` being typed to any.

Fixes #31556.

PR Close #33997
@mhevery mhevery closed this in a6b6d74 Dec 2, 2019
mhevery added a commit that referenced this pull request Dec 2, 2019
Fixes the content of `NgIf` being typed to any.

Fixes #31556.

PR Close #33997
crisbeto added a commit to crisbeto/angular that referenced this pull request Dec 3, 2019
This is a follow-up to angular#33997 where some new generic parameters were added without defaults which is technically a breaking change. These changes add the defaults.
crisbeto added a commit to crisbeto/angular that referenced this pull request Dec 3, 2019
This is a follow-up to angular#33997 where some new generic parameters were added without defaults which is technically a breaking change. These changes add the defaults.
crisbeto added a commit to crisbeto/angular that referenced this pull request Dec 3, 2019
This is a follow-up to angular#33997 where some new generic parameters were added without defaults which is technically a breaking change. These changes add the defaults.
mhevery added a commit that referenced this pull request Dec 4, 2019
This is a follow-up to #33997 where some new generic parameters were added without defaults which is technically a breaking change. These changes add the defaults.

PR Close #34206
mhevery added a commit that referenced this pull request Dec 4, 2019
This is a follow-up to #33997 where some new generic parameters were added without defaults which is technically a breaking change. These changes add the defaults.

PR Close #34206
@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Dec 17, 2019

It seems like there should be a test for how this typing behaves with *ngIf="something | async; let thing". In that case, the behavior here doesn't seem to align with one of the most common use cases of *ngIf.

@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 17, 2020

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.