feat(module:table): handle CDK DataSource for virtual scroll #3766

lppedd · 2019-07-10T15:17:45Z

Opening this PR for discussion only at the moment.
This is the minimal implementation to support the use of a DataSource.
I need this functionality for a product I'm developing and this would be sufficient at the moment.
I'm especially worried about the problem described in this comment.

What is missing

Skeleton while loading data.
Example:
~~Warning when using incompatible inputs (e.g. DataSource and non-virtual scroll)~~

As a next step, I'd like to leverage the DataSource itself, to produce a uniform implementation.
This means if a user passes in [nzData]="[...]", I'll wrap it around an ArrayDataSource.
This would mean an overall cleaner implementation, maybe delegating some functionalities to a common, base DataSource.

Closes #3571

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NG-ZORRO/ng-zorro-antd/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[X] No

codecov · 2019-07-11T13:59:34Z

Codecov Report

❗ No coverage uploaded for pull request base (master@b758572). Click here to learn what that means.
The diff coverage is 81.6%.

@@            Coverage Diff            @@
##             master    #3766   +/-   ##
=========================================
  Coverage          ?   95.15%           
=========================================
  Files             ?      730           
  Lines             ?    14744           
  Branches          ?     1921           
=========================================
  Hits              ?    14030           
  Misses            ?      267           
  Partials          ?      447

Impacted Files	Coverage Δ
components/table/public-api.ts	`100% <100%> (ø)`
components/table/nz-data-source.ts	`71.42% <71.42%> (ø)`
components/table/nz-table.component.ts	`90.96% <79.31%> (ø)`
components/table/nz-empty-data-source.ts	`80% <80%> (ø)`
components/table/nz-array-data-source.ts	`83.33% <83.33%> (ø)`
components/table/nz-detached-data-source.ts	`86.36% <86.36%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b758572...8a4d9f8. Read the comment docs.

lppedd · 2019-07-11T16:11:05Z

As of now builds fail because in other sources you're accessing the table data field directly, e.g.

data.length > 0

This won't work because DataSource doesn't, obviously, have that field.
I'll create a new abstract class NzDataSource, which mimic the length field.
This will also mean abstracting ourself from the CDK one, and it means we can introduce new code without breaking changes in the future.

components/table/style/index.less

vthinkxie

I added some comments, thanks for your PR!

lppedd · 2019-07-15T12:48:08Z

@vthinkxie Could you give a look at the NzTableComponent implementation?
Specifically at the handleDataChange method and the used classes. What do you think? This is temporary but it is a "road" a like.
The way NzDataSource is built means it is compatible with the old usages, where users would take the data field from the table instance to produce tr s.
Also, now we can have both virtual & normal, with both a DataSource and an array.
So basically an user can do whatever he/she wants.

Also now you can check the result in the preview! 😎 I'm happy

Edit: this problem can be solved by incrementing nzVirtualMinBufferPx.

I've also noticed an issue, which was already there, when using virtual scroll with front-end pagination.

It seems the bigger the view is, the most time it occurs.

netlify · 2019-07-15T12:52:05Z

Deploy preview for ng-zorro-master ready!

Built with commit 7b5b12f

https://deploy-preview-3766--ng-zorro-master.netlify.com

netlify · 2019-07-15T13:06:33Z

Deploy preview for ng-zorro-master ready!

Built with commit 8a4d9f8

https://deploy-preview-3766--ng-zorro-master.netlify.com

lppedd · 2019-07-17T16:39:23Z

A quick update.
I have tried many different ways to maintain backward-compatibility without going crazy, but this seems to be the only reasonable one (if you want look at my table-manager branch).
I have re-organized/separated files so it's more clear what I have done.

components/table/nz-table.component.ts

components/table/nz-table.component.less

vthinkxie · 2019-07-27T03:24:56Z

everything looks good to me, @wendzhue @hsuanxyz plz take a look at this
thanks a lot to @lppedd you did a great job!

lppedd · 2019-07-27T09:33:40Z

Thanks!

@vthinkxie @wendzhue @hsuanxyz
I think we need to come up with an idea to better solve the following use-case:

DataSource
front pagination
non-virtual

Which means

<nz-table #table [nzData]="dataSource" [nzFrontPagination]="true" [nzShowPagination]="true">
   ...
   <tr *ngFor="let data of table.data; index as i">
      ...
   </tr>

This is particularly tricky.

The ideal workflow would be:

user asks for a page, let's say page 2.
DataSource is notified, does an HTTP calls, update the internal array
change detection runs and calls [Symbol.iterator]
the table view is updated

However what actually happens is

user asks for a page, let's say page 2.
following the button click, change detection runs (thanks Zone.js!) and calls [Symbol.iterator]
table view is updated
DataSource is notified, does an HTTP calls, update the internal array

You can clearly see there is a problem, the table is "behind" of an HTTP call, and the data displayed is wrong.

Possible solutions:

User must use nzCurrentPageDataChange | async instead of data

 <tr *ngFor="let data of table.nzCurrentPageDataChange | async; index as i">

This plays awesome with this piece of code

private handleDataChange(nzData: NzDataSource<T> | T[]): void {
  ...
  if (this.nzFrontPagination) {
    this.data = new NzDetachedDataSource(this, this.data);
    this.data.connect(this).subscribe({
      next: data => this.nzCurrentPageDataChange.emit(data as T[]) // Here data is emitted
    });
  }
}

User inject ChangeDetectorRef inside its own NzDataSource

private fetchRange(range: ListRange): void {
   this.http
       .get(`...`)
       .subscribe(result => {
         this.data = result;
         this.cdr.detectChanges(); // Here
       });
}

Two change detection cycles are run then.

We wrap the pagination button click in a non-Zone.js area, and somehow we invoke change detection.
We simly say "you cannot do that as of now"
Any other idea? 😄

Here the major issue is backward campatibility. While I like the code I've written, I think it still has limitations.

The thing is getting data using this technique

<tr *ngFor="let data of table.data; index as i">

while pretty neat, require accessing the internal data field. If we wouldn't have had this behavior, I think we could have come up with something better.
But that's just my opinion.

Sorry for the long message, but I prefer having a clear overview of what doesn't work as expected

components/table/nz-table.component.ts

lppedd · 2019-07-28T13:58:22Z

components/table/nz-table.component.ts

    if (this.nzFrontPagination) {
-      this.nzTotal = data.length;
+      this.nzTotal = this.data.length;


This is not ideal, as the NzDataSource instance might not have the value available at this point. I need to refactor this, somehow, into an "asynchronous" process

agree, let's do this :)

components/table/nz-table.component.ts

vthinkxie · 2019-08-09T03:20:10Z

Hi @lppedd
sorry for the late reply
I'm a little confused about the http calls with [nzFrontPagination]="true"

When the nzFrontPagination is true, there should no http calls I think, because all data are loaded at the very beginning, and the pagination split the data into pieces, and display them in the let data of table.data

lppedd · 2019-08-09T07:41:29Z

@vthinkxie but how do you enforce the user to provide a DataSource which simply contains an Array? I mean, as far as I understood, the difference between frontPagination=true and ...=false, is that when true, the NzTableComponent handle the data splitting, while with false, events signaling pages and sizes are emitted, and the user must handle them himself.

The same applies with the DataSource, but it still can have HTTP calls.
Anyway, wouldn't it be the same with a DataSource containing only an Array? It's still asynchronous (sort of) by using Observables (see NzArrayDataSource).

But maybe I misunderstood what you're trying to say.

vthinkxie · 2019-08-09T08:24:26Z

@lppedd
I mean if the user set frontPagination = true, why he still needs to send http calls when clicking pagination?

lppedd · 2019-08-09T08:31:53Z

@lppedd well the user isn't forced to do HTTP calls, but it still is an option. We don't have control over the implementation of the inputted DataSource.
What I do when frontPagination = true, is signal page changes to the DataSource with a ListRange event.

vthinkxie · 2019-08-09T08:43:53Z

@lppedd
yes, frontPagination = true should work with virtual scroll if user wants to send http calls, right?
if so I think we can add a note in the document, to tell the user not using frontPagination = true with non-virtual-scroll and I think most users would not use it in most scenarios

lppedd · 2019-08-09T08:52:00Z

@vthinkxie yes absolutely. DataSource + frontPagination = true + virtual scroll, works nicely always.

Just a question: as I had written in the long message above, why not deprecating the use of the internal field data in favor of an event, such as nzCurrentPageDataChange.
This doesn't solve the problem, but at least is a step forward.

Anyway, I'll review the code again, and report back if I find something else worth discussing (I don't think so). Then I'll write some tests.
There is #3766 (comment) left off.

vthinkxie · 2019-08-09T08:59:37Z

@lppedd
yes, we should move a step forward, but I think most current user works fine with Array input, let's talk about the breaking change when 9.0 comes.

lppedd · 2019-08-09T09:02:45Z

@vthinkxie sure, agree. I'll keep you updated also on #3766 (comment), which needs some more thinking. If you want to throw ideas of any kind, they're very welcome.

lppedd · 2019-09-09T20:14:39Z

@vthinkxie Sorry I've been pretty busy with other work. Just wanted to let you know I haven't forgotten.

vthinkxie · 2020-03-24T12:35:40Z

Hi @lppedd
the table component are refactored in v9 now, and all data process are moved to table-data.service.ts now, it should be much clearer than before
feel free to ping me if you need any help

vthinkxie · 2020-06-11T02:54:09Z

Hi
This Pull Request is going to be closed since no update for a long time, feel free to open a new PR if any new update.

pr-triage bot added the PR: unreviewed label Jul 10, 2019

lppedd force-pushed the master branch 2 times, most recently from 7928f05 to 02da46a Compare July 11, 2019 15:31

lppedd mentioned this pull request Jul 11, 2019

NzTableComponent should support CDK Datasource #3571

Open

vthinkxie reviewed Jul 12, 2019

View reviewed changes

components/table/style/index.less Outdated Show resolved Hide resolved

vthinkxie requested changes Jul 15, 2019

View reviewed changes

pr-triage bot added PR: reviewed-changes-requested PR: unreviewed and removed PR: unreviewed PR: reviewed-changes-requested labels Jul 15, 2019

lppedd force-pushed the master branch 2 times, most recently from 844c211 to c68c4f0 Compare July 15, 2019 12:47

lppedd force-pushed the master branch 4 times, most recently from 703ef69 to a2d09d7 Compare July 17, 2019 16:36

lppedd force-pushed the master branch 2 times, most recently from 0320865 to b4ab3bf Compare July 17, 2019 19:41

vthinkxie reviewed Jul 18, 2019

View reviewed changes

components/table/nz-table.component.ts Outdated Show resolved Hide resolved

lppedd force-pushed the master branch from b4ab3bf to 386b962 Compare July 18, 2019 08:50

lppedd mentioned this pull request Jul 18, 2019

build(module:*): strip internal-annotated declarations #3814

Merged

3 tasks

lppedd force-pushed the master branch 3 times, most recently from 6500a6c to 246a678 Compare July 18, 2019 16:45

vthinkxie requested review from wzhudev and hsuanxyz July 27, 2019 03:20

vthinkxie reviewed Jul 27, 2019

View reviewed changes

components/table/nz-table.component.less Outdated Show resolved Hide resolved

lppedd force-pushed the master branch 2 times, most recently from ba541b0 to cfdd93c Compare July 27, 2019 12:24

lppedd commented Jul 28, 2019

View reviewed changes

components/table/nz-table.component.ts Outdated Show resolved Hide resolved

lppedd commented Jul 28, 2019

View reviewed changes

components/table/nz-table.component.ts Outdated Show resolved Hide resolved

lppedd commented Jul 28, 2019

View reviewed changes

components/table/nz-table.component.ts Outdated Show resolved Hide resolved

Edoardo Luppi and others added 4 commits July 28, 2019 18:37

feat(module:table): handle CDK DataSource for virtual scroll

a0eab86

refactor: move styles from component to patch

36f699c

refactor: complete generic typing for NzDetachedDataSource's constructor

f2ffe9e

refactor: simplify code

8a4d9f8

lppedd force-pushed the master branch from e865f09 to 8a4d9f8 Compare July 28, 2019 16:37

lppedd mentioned this pull request Aug 9, 2019

Table: 虚拟滚动渲染问题 #3948

Closed

lppedd mentioned this pull request Oct 19, 2019

nz-table nzVirtualScroll last item rendered is wrong #4151

Closed

vthinkxie closed this Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(module:table): handle CDK DataSource for virtual scroll #3766

feat(module:table): handle CDK DataSource for virtual scroll #3766

lppedd commented Jul 10, 2019 •

edited

codecov bot commented Jul 11, 2019 •

edited

lppedd commented Jul 11, 2019 •

edited

vthinkxie left a comment

lppedd commented Jul 15, 2019 •

edited

netlify bot commented Jul 15, 2019

netlify bot commented Jul 15, 2019 •

edited

lppedd commented Jul 17, 2019 •

edited

vthinkxie commented Jul 27, 2019 •

edited

lppedd commented Jul 27, 2019 •

edited

lppedd Jul 28, 2019 •

edited

vthinkxie Aug 9, 2019

vthinkxie commented Aug 9, 2019 •

edited

lppedd commented Aug 9, 2019 •

edited

vthinkxie commented Aug 9, 2019

lppedd commented Aug 9, 2019 •

edited

vthinkxie commented Aug 9, 2019

lppedd commented Aug 9, 2019 •

edited

vthinkxie commented Aug 9, 2019 •

edited

lppedd commented Aug 9, 2019

lppedd commented Sep 9, 2019

vthinkxie commented Mar 24, 2020

vthinkxie commented Jun 11, 2020

feat(module:table): handle CDK DataSource for virtual scroll #3766

feat(module:table): handle CDK DataSource for virtual scroll #3766

Conversation

lppedd commented Jul 10, 2019 • edited

PR Checklist

PR Type

Does this PR introduce a breaking change?

codecov bot commented Jul 11, 2019 • edited

Codecov Report

lppedd commented Jul 11, 2019 • edited

vthinkxie left a comment

Choose a reason for hiding this comment

lppedd commented Jul 15, 2019 • edited

netlify bot commented Jul 15, 2019

netlify bot commented Jul 15, 2019 • edited

lppedd commented Jul 17, 2019 • edited

vthinkxie commented Jul 27, 2019 • edited

lppedd commented Jul 27, 2019 • edited

lppedd Jul 28, 2019 • edited

Choose a reason for hiding this comment

vthinkxie Aug 9, 2019

Choose a reason for hiding this comment

vthinkxie commented Aug 9, 2019 • edited

lppedd commented Aug 9, 2019 • edited

vthinkxie commented Aug 9, 2019

lppedd commented Aug 9, 2019 • edited

vthinkxie commented Aug 9, 2019

lppedd commented Aug 9, 2019 • edited

vthinkxie commented Aug 9, 2019 • edited

lppedd commented Aug 9, 2019

lppedd commented Sep 9, 2019

vthinkxie commented Mar 24, 2020

vthinkxie commented Jun 11, 2020

lppedd commented Jul 10, 2019 •

edited

codecov bot commented Jul 11, 2019 •

edited

lppedd commented Jul 11, 2019 •

edited

lppedd commented Jul 15, 2019 •

edited

netlify bot commented Jul 15, 2019 •

edited

lppedd commented Jul 17, 2019 •

edited

vthinkxie commented Jul 27, 2019 •

edited

lppedd commented Jul 27, 2019 •

edited

lppedd Jul 28, 2019 •

edited

vthinkxie commented Aug 9, 2019 •

edited

lppedd commented Aug 9, 2019 •

edited

lppedd commented Aug 9, 2019 •

edited

lppedd commented Aug 9, 2019 •

edited

vthinkxie commented Aug 9, 2019 •

edited