Skip to content

fix(paginator): update page index if invalid after length change - #14625

Open
crisbeto wants to merge 1 commit into
angular:mainfrom
crisbeto:14520/paginator-index-update
Open

fix(paginator): update page index if invalid after length change#14625
crisbeto wants to merge 1 commit into
angular:mainfrom
crisbeto:14520/paginator-index-update

Conversation

@crisbeto

Copy link
Copy Markdown
Member

Updates the page index of a paginator if its length changed to something where the index isn't valid anymore (e.g. the user is on the last page and the amount of pages becomes smaller).

Fixes#14520.

@crisbetocrisbeto added the target: patch This PR is targeted for the next patch release label Dec 24, 2018
@googlebotgooglebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 24, 2018
const currentPageIndex = this._pageIndex;

if (currentPageIndex > maxPageIndex && this.initialized) {
// Needs to happen on the next tick, in order to avoid "changed after checked" errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why does this cause a changed after checked error?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's been a while so I don't remember the exact reason. I think it had to do with the page event being emitted at the wrong time the which would cause the table to be re-rendered after the changes were detected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel iffy about just throwing a microtask at it, especially without a clear explanation of why it's necessary.

@mmalerbammalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguinandrewseguin added the P4 A relatively minor issue that is not relevant to core functions label May 30, 2019
@crisbeto
crisbetoforce-pushed the 14520/paginator-index-update branch from 534af71 to 9febcbeCompareJuly 3, 2019 21:45
@crisbeto
crisbetoforce-pushed the 14520/paginator-index-update branch from 9febcbe to 1b3c9f8CompareFebruary 9, 2020 16:44

@andrewseguinandrewseguin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Little concerned about timing on this - could this interfere if the user tries to change the index and length at the same time? E.g. if the user changes the index + length, and the length gets set first, then a promise gets started that will change the index even though the user will have already changed it at that point

@thw0rted

Copy link
Copy Markdown

I've been looking at @andrewseguin 's comments and I think that concern would be addressed by moving L136 to L130 (and L139 to L141, so that all the new code happens inside of the then callback. The potential race condition arises because you're computing maxPageIndex and looking at the current _pageIndex value during the first tick but changing them during the second. If you do the comparison and change in a single tick, the race is eliminated.

@nvaralakshmi

Copy link
Copy Markdown

@crisbeto it looks like your PR has fallen out of date please rebase the PR.

…ange
Updates the page index of a paginator if its length changed to something where the index isn't valid anymore (e.g. the user is on the last page and the amount of pages becomes smaller).
Fixesangular#14520.
@crisbeto

Copy link
Copy Markdown
MemberAuthor

I've rebased the PR.

@crisbeto
crisbetoforce-pushed the 14520/paginator-index-update branch from 1b3c9f8 to 339c2ebCompareNovember 23, 2021 10:33
@andrewseguinandrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@andrewseguinandrewseguin removed the P4 A relatively minor issue that is not relevant to core functions label Mar 28, 2022
@josephperrottjosephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@josephperrott
josephperrott requested a review from a team as a code ownerDecember 18, 2024 17:40
@josephperrott
josephperrott requested review from amysorto and mmalerba and removed request for a teamDecember 18, 2024 17:40
@mmalerba
mmalerba removed their request for review February 19, 2025 00:40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: cleanupThe PR is in need of cleanup, either due to needing a rebase or in response to comments from reviewstarget: patchThis PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Paginator] When Updated length is "shorter" than the pageIndex (*pageSize), pageIndex should be updated and then a pageEvent to the pageIndex emited

10 participants

@crisbeto@thw0rted@nvaralakshmi@jelbourn@andrewseguin@devversion@googlebot@amysorto@josephperrott@mmalerba