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

feat(cdk/scrolling): improve template type checking for CdkVirtualForOf Directive #26609

Closed
windmichael opened this issue Feb 12, 2023 · 7 comments · Fixed by #27276
Closed
Labels
area: cdk/scrolling feature This issue represents a new feature or feature request rather than a bug or bug fix help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@windmichael
Copy link

Feature Description

Feature Description

The CdkVirtualForOf Directive in the "@angular/cdk/scrolling" module does not implement the ng-template context guard, as it is done at the *ngFor Structual Directive in the Angular Common package.
Thus, when using the *cdkVirtualFor Structual Directive in an HTML Template, the type of the item is "any".

Expample:

<cdk-virtual-scroll-viewport itemSize="50" class="example-viewport">
  <div *cdkVirtualFor="let item of items" class="example-item">{{ item }}</div>
</cdk-virtual-scroll-viewport>

Although the type of the property "items" is "string[]", the type of "item" is "any".

Expected behavior

The type of the property "item" should be "string" when the type of "items" is "string[]".

Suggested solution

Implement the static method "ngTemplateContextGuard" for the CdkVirtualForOf Directive.

public static ngTemplateContextGuard<T>(
    directive: CdkVirtualForOf<T>,
    context: unknown,
  ): context is CdkVirtualForOfContext<T> {
    return true;
  }

https://angular.io/guide/structural-directives#typing-the-directives-context

If you want, I can create a PR therefore, because I tried this solution already.

Use Case

No response

@windmichael windmichael added feature This issue represents a new feature or feature request rather than a bug or bug fix needs triage This issue needs to be triaged by the team labels Feb 12, 2023
@mmalerba mmalerba added help wanted The team would appreciate a PR from the community to address this issue area: cdk/scrolling and removed needs triage This issue needs to be triaged by the team labels May 19, 2023
@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jun 1, 2023
naaajii added a commit to naaajii/components that referenced this issue Jun 11, 2023
implements ngTemplateContextGuard for
CdkVirtualForOf directive

fixes angular#26609
naaajii added a commit to naaajii/components that referenced this issue Jun 11, 2023
implements ngTemplateContextGuard for CdkVirtualForOf directive

fixes angular#26609
@distante
Copy link

Still an Issue

@mstawick
Copy link

mstawick commented Dec 1, 2023

If anyone wants a workaround, I was suggested one here:
https://youtrack.jetbrains.com/issue/WEB-64040/Angular-template-broken-type-inference-for-cdkVirtualFor-2023.3-Beta

basically add:
static ngTemplateContextGuard<T>(dir: CdkVirtualForOf<T>, ctx: any): ctx is CdkVirtualForOfContext<T>;

in the @angular/cdk/scrolling/index.d.ts file for the:

export declare class CdkVirtualForOf<T> implements CdkVirtualScrollRepeater<T>, CollectionViewer, DoCheck, OnDestroy

declaration.

@medchat-layton
Copy link

@mstawick but this just gets completely blown away whenever we update packages right? Fixes like this just drop through the cracks and can't be considered safe in any way, as far as I can tell :(

@PhOder
Copy link

PhOder commented Dec 30, 2023

@medchat-layton I know this might not be ideal but for workarounds like this I like to use https://github.com/ds300/patch-package to create a diff of such workarounds that you can add to your repository.

On npm install these patches will be automatically applied to the node_modules folder.

@mstawick
Copy link

@medchat-layton I completely agree with you, that's why I used the word "workaround". Given current state, not having proper type inference in templates destroys my productivity, so I'd rather apply the workaround after update if I have to, rather then have no workaround at all.

@jakubgosk
Copy link

I'm trying to figure it out in a way that would not require changing library files. In the past I managed to achive such thing for matCellDef with directive, so I tried that method here as well and came up with this:

@Directive({
  selector: '[cdkVirtualFor]',
  standalone: true,
})
export class TypeSafeCdkVirtualForDirective<T> {
  @Input() cdkVirtualForDataSource: T[];

  static ngTemplateContextGuard<T>(
    _dir: TypeSafeCdkVirtualForDirective<T>,
    ctx: unknown,
  ): ctx is CdkVirtualForOfContext<T> {
    return true;
  }
}

and then you simply change cdkVirtualFor to:
*cdkVirtualFor="let customer of data; dataSource: data"

It's not perfect but for me it gets job done.

crisbeto pushed a commit that referenced this issue Sep 2, 2024
BREAKING CHANGE:
* Virtual scrolling lists now have proper type checking which can reveal some previously-hidden compilation errors.

* fix(cdk/scrolling): adds ngTemplateContextGuard

implements ngTemplateContextGuard for CdkVirtualForOf directive

fixes #26609

* fixup! fix(cdk/scrolling): adds ngTemplateContextGuard
@angular-automatic-lock-bot
Copy link

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 Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: cdk/scrolling feature This issue represents a new feature or feature request rather than a bug or bug fix help wanted The team would appreciate a PR from the community to address this issue P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants