Skip to content

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crisbeto
Copy link
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.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Dec 24, 2018
@googlebot googlebot 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
Member

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
Member Author

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
Member

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.

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguin andrewseguin added the P4 A relatively minor issue that is not relevant to core functions label May 30, 2019
@crisbeto crisbeto force-pushed the 14520/paginator-index-update branch from 534af71 to 9febcbe Compare July 3, 2019 21:45
@crisbeto crisbeto force-pushed the 14520/paginator-index-update branch from 9febcbe to 1b3c9f8 Compare February 9, 2020 16:44
Copy link
Contributor

@andrewseguin andrewseguin left a comment

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

thw0rted commented Aug 6, 2021

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

@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).

Fixes angular#14520.
@crisbeto
Copy link
Member Author

I've rebased the PR.

@crisbeto crisbeto force-pushed the 14520/paginator-index-update branch from 1b3c9f8 to 339c2eb Compare November 23, 2021 10:33
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@andrewseguin andrewseguin removed the P4 A relatively minor issue that is not relevant to core functions label Mar 28, 2022
@josephperrott josephperrott 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 owner December 18, 2024 17:40
@josephperrott josephperrott requested review from amysorto and mmalerba and removed request for a team December 18, 2024 17:40
@mmalerba mmalerba removed their request for review February 19, 2025 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release
Projects
None yet
10 participants