8000 feat(template): add DataSource support for rx-virtual-for directive by friendlyAce · Pull Request #1764 · rx-angular/rx-angular · GitHub
[go: up one dir, main page]
Skip to content

feat(template): add DataSource support for rx-virtual-for directive #1764

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(template): add DataSource support for rx-virtual-for directive #1764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9b9bdd
feat(template): add DataSource support for rx-virtual-for directive
friendlyAce Aug 31, 2024
09270f9
fix(template): improve DataSource lifecycle management in RxVirtualFor
friendlyAce May 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libs/template/experimental/virtual-scrolling/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
export {
CollectionViewer,
DataSource,
ListRange,
RxVirtualForViewContext,
RxVirtualScrollElement,
Expand Down
11 changes: 11 additions & 0 deletions libs/template/experimental/virtual-scrolling/src/lib/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,17 @@ export interface ListRange {
end: number;
}

export abstract class DataSource<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know if it's possible to maybe have an import type { DataSource } from '@angular/cdk' without having it to mention as peerDependency?

That would be the absolute best case

Copy link
Contributor Author
@friendlyAce friendlyAce May 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoebbelsB Hmm im not fully sure on that, i tried to have a bit of testing on that but i kept running into a bunch of possibly unrelated issues and stopped checking.

My idea was to use Typescripts "type only imports" e. g. import type { DataSource } from '@angular/cdk/collections (doc)
As when compiling they should just be dropped out of the dist output files
For that i reverted the DataSource and CollectionViewer declarations which i added in

diff --git a/libs/template/experimental/virtual-scrolling/src/index.ts b/libs/template/experimental/virtual-scrolling/src/index.ts
index 86b34086..8c50f1ed 100644
--- a/libs/template/experimental/virtual-scrolling/src/index.ts
+++ b/libs/template/experimental/virtual-scrolling/src/index.ts
@@ -1,6 +1,4 @@
 export {
-  CollectionViewer,
-  DataSource,
   ListRange,
   RxVirtualForViewContext,
   RxVirtualScrollElement,
diff --git a/libs/template/experimental/virtual-scrolling/src/lib/model.ts b/libs/template/experimental/virtual-scrolling/src/lib/model.ts
index c6ed5c35..aacb1d3e 100644
--- a/libs/template/experimental/virtual-scrolling/src/lib/model.ts
+++ b/libs/template/experimental/virtual-scrolling/src/lib/model.ts
@@ -34,17 +34,6 @@ export interface ListRange {
   end: number;
 }
 
-export abstract class DataSource<T> {
-  abstract connect(
-    collectionViewer: CollectionViewer,
-  ): Observable<NgIterable<T>>;
-  abstract disconnect(collectionViewer: CollectionViewer): void;
-}
-
-export interface CollectionViewer {
-  viewChange: Observable<ListRange>;
-}
-
 /**
  * @Directive RxVirtualScrollStrategy
  *
diff --git a/libs/template/experimental/virtual-scrolling/src/lib/virtual-for.directive.ts b/libs/template/experimental/virtual-scrolling/src/lib/virtual-for.directive.ts
index 174faf3e..14ee2512 100644
--- a/libs/template/experimental/virtual-scrolling/src/lib/virtual-for.directive.ts
+++ b/libs/template/experimental/virtual-scrolling/src/lib/virtual-for.directive.ts
@@ -56,8 +56,6 @@ import {
   tap,
 } from 'rxjs/operators';
 import {
-  CollectionViewer,
-  DataSource,
   ListRange,
   RxVirtualForViewContext,
   RxVirtualScrollStrategy,
@@ -67,6 +65,10 @@ import {
   createVirtualListTemplateManager,
   RxVirtualListTemplateManager,
 } from './virtual-list-template-manager';
+import type {
+  DataSource,
+  CollectionViewer
+} from '@angular/cdk/collections';
 import {
   DEFAULT_TEMPLATE_CACHE_SIZE,
   RX_VIRTUAL_SCROLL_DEFAULT_OPTIONS,

And instead i used the type only imports directly in virtual-for.directive.ts, e. g.

import type {
  DataSource,
  CollectionViewer
} from '@angular/cdk/collections';

Therefore, at least for the library build, the @angular/cdk package needs to be resolvable, which it is, as its already part of your package.json dependencies here.

After these changes i ran the build for the template library via npx nx run template:build:production, which gave me the according dist output dist\libs\template.

Taking a short look at the compiled .mjs file, indeed the import from @angular/cdk is NOT mentioned (as its a type import)
dist\libs\template\fesm2022\template-experimental-virtual-scrolling.mjs

But when having a look at the compiled type declaration file:
dist\libs\template\experimental\virtual-scrolling\lib\virtual-for.directive.d.ts
It still contains the @angular/cdk import...
import type { DataSource } from '@angular/cdk/collections';
image

Which could cause an issue for library consumers.

So i wanted to check this and therefore created a new nx angular workspace via
npx create-nx-workspace demo-workspace --preset=angular
And installed the local dist template library via
npm i ../rx-angular/dist/libs/template
package.json => "@rx-angular/template": "file:../rx-angular/dist/libs/template"
I also ensured that the @angular/cdk package is not installed in this new demo-workspace project

Then i creating a small dummy component (similar to this example), that uses the virtual for directive, the viewport etc. to see if the serve and build of that demo application would work or if i would run into some errors related to the missing angular/cdk peer dependency.

But unfortunately my serve had a bunch of unrelated errors in regards of type conflicts of rxjs

Type 'import("DEMO-WORKSPACE/node_modules/rxjs/dist/types/internal/Observable").Observable<any> | undefined' is not assignable to type 'import("rx-angular/node_modules/rxjs/dist/types/internal/Observable").Observable<any> | undefined'.
Types of property 'operator' are incompatible.

Which is a classic issue when using local packages, as the workspaces have their own node_modules directories with their own rxjs installations, which TS sees as incompatible
So instead i used npm links

npm uninstall @rx-angular/cdk @rx-angular/template
cd rx-angular/dist/libs/template
npm link
cd demo-workspace
npm link @rx-angular/template

$ npm ls -g --link=true
C:\Users...\AppData\Roaming\npm
└── @rx-angular/template@19.2.2 -> rx-angular\dist\libs\template

alternatively, removing the rx-angular/node_modules directory also works.

But nevertheless when serving i keep getting this error.
image

So either something is broken with the local build of the template lib, or something else. After trying a bunch of stuff, i couldn't get this fixed and therefore couldn't properly check if this approach would work for library consumers.

But i would probably still expect, something like "Cannot find module '@angular/cdk/collections' or its corresponding type declarations" when somebody uses the rx-angular/template library, without them having the angular cdk package also installed. And therefore i think the peer dependency to ng cdk would need to be specified in your lib

abstract connect(
collectionViewer: CollectionViewer,
): Observable<NgIterable<T>>;
abstract disconnect(collectionViewer: CollectionViewer): void;
}

export interface CollectionViewer {
viewChange: Observable<ListRange>;
}

/**
* @Directive RxVirtualScrollStrategy
*
Expand Down
68 changes: 61 additions & 7 deletions libs/template/experimental/virtual-scrolling/src/lib/virtual-for.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { Promise } from '@rx-angular/cdk/zone-less/browser';
import {
combineLatest,
concat,
ConnectableObservable,
isObservable,
MonoTypeOperatorFunction,
NEVER,
Expand All @@ -55,6 +56,8 @@ import {
tap,
} from 'rxjs/operators';
import {
CollectionViewer,
DataSource,
ListRange,
RxVirtualForViewContext,
RxVirtualScrollStrategy,
Expand Down Expand Up @@ -213,6 +216,11 @@ export class RxVirtualFor<T, U extends NgIterable<T> = NgIterable<T>>
optional: true,
});

/** @internal */
private connectedDataSource?: DataSource<T>;
/** @internal */
private collectionViewer?: CollectionViewer;

/** @internal */
private _differ?: IterableDiffer<T>;

Expand All @@ -234,33 +242,69 @@ export class RxVirtualFor<T, U extends NgIterable<T> = NgIterable<T>>
* [hero]="hero"></app-hero>
* </rx-virtual-scroll-viewport>
*
* @param potentialSignalOrObservable
* @param potentialSignalOrObservableOrDataSource
*/
@Input()
set rxVirtualForOf(
potentialSignalOrObservable:
potentialSignalOrObservableOrDataSource:
| Observable<(U & NgIterable<T>) | undefined | null>
| Signal<(U & NgIterable<T>) | undefined | null>
| DataSource<T>
| (U & NgIterable<T>)
| null
| undefined,
) {
if (isSignal(potentialSignalOrObservable)) {
if (isSignal(potentialSignalOrObservableOrDataSource)) {
this.staticValue = undefined;
this.renderStatic = false;
this.observables$.next(
toObservable(potentialSignalOrObservableOrDataSource, {
injector: this.injector,
}),
);
} else if (this.isDataSource(potentialSignalOrObservableOrDataSource)) {
this.disconnectDataSource();

this.staticValue = undefined;
this.renderStatic = false;

const collectionViewer: CollectionViewer = {
viewChange: this.scrollStrategy.renderedRange$,
};

this.collectionViewer = collectionViewer;
this.connectedDataSource = potentialSignalOrObservableOrDataSource;

this.observables$.next(
toObservable(potentialSignalOrObservable, { injector: this.injector }),
potentialSignalOrObservableOrDataSource.connect(collectionViewer),
);
} else if (!isObservable(potentialSignalOrObservable)) {
this.staticValue = potentialSignalOrObservable;
} else if (!isObservable(potentialSignalOrObservableOrDataSource)) {
this.staticValue = potentialSignalOrObservableOrDataSource;
this.renderStatic = true;
} else {
this.staticValue = undefined;
this.renderStatic = false;
this.observables$.next(potentialSignalOrObservable);
this.observables$.next(potentialSignalOrObservableOrDataSource);
}
}

/** @internal */
private isDataSource(
value:
| (U & NgIterable<T>)
| Observable<U & NgIterable<T>>
| DataSource<T>
| null
| undefined,
): value is DataSource<T> {
return (
value != null &&
'connect' in value &&
typeof value.connect === 'function' &&
!(value instanceof ConnectableObservable)
);
}

/**
* @internal
* A reference to the template that is created for each item in the iterable.
Expand Down Expand Up @@ -640,8 +684,18 @@ export class RxVirtualFor<T, U extends NgIterable<T> = NgIterable<T>>
}
}

/** @internal */
private disconnectDataSource(): void {
if (this.connectedDataSource && this.collectionViewer) {
this.connectedDataSource.disconnect(this.collectionViewer);
this.connectedDataSource = undefined;
this.collectionViewer = undefined;
}
}

/** @internal */
ngOnDestroy() {
this.disconnectDataSource();
this._destroy$.next();
this.templateManager.detach();
}
Expand Down
Loading
0