fix(paginator): not emitting page event when pageIndex, pageSize or length changes

[crisbeto]

Currently the paginator won't fire the page event if it's pageIndex, length or pageSize change, which means that the associated table won't be able to react. These changes emit the event if the values change.

Fixes #12583.

[jelbourn]

Should this also markForCheck?

[crisbeto]

I don't think it needs to since it's going through an Output. At least in my testing it wasn't necessary.

[josephperrott]

Since alot of usages of the MatPaginator with a DataSource update the length whenever the data updates, this could cause a loop of sorts.

The if block would prevent it from happening more than once, but we still would be triggering the DataSource's value multiple times.

An example can be seen here with the table-data-source.

[crisbeto]

Doesn't the table user the IterableDiffer under the hood? That should prevent it from doing extra work even if it does fire multiple times. I'm fine with cutting down the PR so it only addresses the pageSize as mentioned in #12583, but I can imagine people running into the same issue for the other two properties.

[crisbeto]

We just got another issue about the pageIndex: #12620.

[jelbourn]

Something does feel weird about this change. Andrew would have a better idea, but I vaguely recall the decision for page events to emit on user interaction being intentional. The thinking was probably that if the user is setting new values into the paginator, they obviously already know about those values and should also propagate them to wherever else they're needed. This makes the page event consistent with change events from other components, which also do this, mirroring native html controls.

[crisbeto]

I suppose the difference between the page event and the various change events is that page is also being used internally inside the table data source in order to know when to re-render the data.

[jelbourn]

Mind if we sit on this one for a few weeks until Andrew gets back? I'd like to chat with him about it.

[ngbot]

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[dchennells]

The page index is typically set to zero in user code in response to sort changes and filter changes, which typically themselves already result in table-reloading chains of events including calls to a service or repository. (While an application-user might be half-way through a long paginated list, they are generally assumed to want to be placed at the top of the reformulated list when the sort or filter changes appear. Sort and filter changes render page position meaningless or at least ambiguous; and, by convention, sorting is seldom implemented on a paginated-page-local basis.) If MatPaginator.page fires as a result of this reset in code to the first page, a second, redundant call on the datasource would be caused under common circumstances, unless user code implements some sort of gating or temporal windowing/debouncing mechanism.

Overall, this change might cause some inconvenience for alert developers, and, a subtle bandwidth and performance loss for less-alert developers. (Subtle, because it will be triggered only if the page number is not already zero at the time the filter or sort change is made; we are talking about an extra, back-end call in SPAs; and, finally, it might not jump off the page for some devs less familiar with asynchronous reactive subscription lambda syntax.) If this breaking change is to be made, then perhaps it should be mitigated by adding a new MatPaginator method that goes to the first page without triggering the MatPaginator.page event and to disseminate knowledge (e.g. in the examples) that this call should be used in lieu of this.paginator.pageIndex = 0 in this scenario.

For typical current user code, consider the following excerpt from the "Table retrieving data through HTTP" mat-table example in the API docs online.

ngOnInit() {
    this.exampleDatabase = new ExampleHttpDao(this.http);

// If the user changes the sort order, reset back to the first page.
    this.sort.sortChange.subscribe(() => **this.paginator.pageIndex = 0**);

    merge(**this.sort.sortChange, this.paginator.page**)
      .pipe(
        startWith({}),
        switchMap(() => {
          this.isLoadingResults = true;
          return this.exampleDatabase!.getRepoIssues(
            this.sort.active, this.sort.direction, this.paginator.pageIndex);
        }),
        map(data => {...

(Incidentally, I found this issue when I sought to confirm that, as things stand, I would not have to intercept/block incident calls to my datasource indirectly caused by my component filtering and sort event-handling functions.)

[singulli1]

Any updates on this?

[andrewseguin]

Many of our components have a stateChanges stream that emits when inputs are changed. I think that would be better suited for this case. The MatTableDataSource could watch for changes off that stream and re-render. For users that have their own data source, they can either listen for that stream or otherwise handle what they provide to the table when these inputs are changed.

[mmalerba]

Closing per Andrew's comment above that we would prefer something like a stateChanges stream (or at this point, maybe signals)