Skip to content

fix(common): add generics to NgIf declaration - #25983

Closed
piotrtomiak wants to merge 1 commit into
angular:masterfrom
piotrtomiak:ng-if-generics
Closed

fix(common): add generics to NgIf declaration#25983
piotrtomiak wants to merge 1 commit into
angular:masterfrom
piotrtomiak:ng-if-generics

Conversation

@piotrtomiak

@piotrtomiak piotrtomiak commented Sep 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

This change allows IDE to analyze type of the variable in ngIf context.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

The issue has been originally filed with Webstorm - https://youtrack.jetbrains.com/issue/WEB-31760 .

In the following example code (login$: Observable<Login>):

<div *ngIf="login$ | async as login">
	{{ login. }}
</div>

login type is detected as any and no content assist is available for the variable fields.

What is the new behavior?

It will be possible to provide content assist by IDE on the login fields, similarly to how it's done for ngFor template.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

Consumers of ngIf API through templates will not be affected. It is possible that AoT compilation might expose some issues, but I am not familiar with the process, so it's hard for me to tell.

Other information

If PR is accepted it would be good to release the fix on all currently active release branches, so that users can take benefit from the fix even if they are on older version of Angular.

@trotyl

trotyl commented Sep 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'd suggest:

  • Change fix to feat as this is about enhancement;
  • Change <T> to <T=any> to make it non-breaking (there could be imperative usage);

Also I'm not sure whether this could help language service, or even the compiler, as type-checking for @Input()s not works at all.

@piotrtomiak
feat(common): add generics to NgIf declaration
16f08b3 
This change allows IDE to analyze type of the variable in ngIf context.
@piotrtomiak

piotrtomiak commented Sep 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@trotyl - thanks for suggestions, I've applied them. Webstorm only partially uses language services and on our side we have just implemented support for proper type evaluation of @Input() and template contexts, which is based on types declared in .d.ts files. This change will also benefit language services if such a support is ever added.

@kara kara added the area: common Issues related to APIs in the @angular/common package label Sep 22, 2018
@piotrtomiak

piotrtomiak commented Sep 24, 2018

Copy link
Copy Markdown
Contributor Author

Is there anything more I should change for the PR to be accepted?

@piotrtomiak piotrtomiak closed this Oct 1, 2018
@piotrtomiak piotrtomiak reopened this Oct 1, 2018
@piotrtomiak

piotrtomiak commented Oct 24, 2018

Copy link
Copy Markdown
Contributor Author

Any plans to include this PR?

@trotyl

trotyl commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

I'm not sure about whether Angular team is scheduling this, but from other discussions, excluding null is not the correct semantic, NgIf is having the same behavior of JavaScript if, namely anything falsy will be excluded.

@piotrtomiak

piotrtomiak commented Nov 19, 2018

Copy link
Copy Markdown
Contributor Author

I've managed to solve this issue differently.

@sod

sod commented Nov 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Funny how a developer from jetbrains tried to contribute so every editor benefits. But had to give up because of no response and baked it directly into intellij. I love angular but this is just heart breaking.

@jotatoledo

jotatoledo commented Nov 22, 2018

Copy link
Copy Markdown
Contributor

This would be a nice to have feat.

@jotatoledo jotatoledo mentioned this pull request Nov 22, 2018
Closed
@piotrtomiak
piotrtomiak deleted the ng-if-generics branch April 3, 2019 13:26
@sod sod mentioned this pull request Apr 9, 2019
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

area: common Issues related to APIs in the @angular/common package cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@piotrtomiak @trotyl @sod @jotatoledo @kara @googlebot