Skip to content
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

fix: make explicit data request when clearing cache with no initial pool #2119

Merged
merged 6 commits into from
Jan 18, 2021

Conversation

yuriy-fix
Copy link
Contributor

@yuriy-fix yuriy-fix marked this pull request as ready for review January 14, 2021 11:05
test/sorting.test.js Outdated Show resolved Hide resolved
test/sorting.test.js Outdated Show resolved Hide resolved
@tomivirkki
Copy link
Member

tomivirkki commented Jan 15, 2021

Did some debugging with the original issue and this might be fixable in the data-provider-mixin:

Screenshot 2021-01-15 at 18 25 47

Seems that the <tbody> is empty, so initial pool has never been created. Added a debugger to increasePoolIfNeeded...

Screenshot 2021-01-15 at 18 26 20

It was never called...

Screenshot 2021-01-15 at 18 27 01

...so added another one before the guard statement. It got caught a couple of times before the panel was opened, but also once after it got opened. All the other conditions were fine but canPopulate() was returning false

Screenshot 2021-01-15 at 18 28 06

The canPopulate() function checks for _hasData and _columnTree and out of those two, it was the _hasData that was falsy...

Screenshot 2021-01-15 at 18 28 32

Seems that _hasData gets set true when a data provider responds...

Screenshot 2021-01-15 at 18 29 14

...so it must have been true at some point.

Screenshot 2021-01-15 at 18 30 02

The only place that sets it false is clearCache(), and sort-mixin indeed calls clearCache() (once the sorters are set).

Screenshot 2021-01-15 at 18 30 34

The end of clearCache() makes an explicit data request, but only in the case the grid doesn't have physical items/rows/effectiveSize beforehand. If it does, then it relies on the grid rows to ask for more data and the explicit data request is skipped.

But the condition here seems to be wrong since even though the grid has _effecticeSize, there are 0 physical rows rendered at this point and thus the new data request never gets made (and _hasData will remain false).

So one fix might be to change the condition to also make the explicit data request in case the initial pool hasn't been created:

Screenshot 2021-01-15 at 18 31 55

The PR's original revision works because it explicitly calls createPool right before the sort-mixin's clearCache manages to change _hasData to false. It's probably a plausible fix in case sorters are used but an explicit call to clearCache() of a hidden grid might still suffer from the same issue (that's probably a good scenario for a test case for the fix). If possible, it's better to fix the root cause of the issues.

@yuriy-fix yuriy-fix changed the title fix: increase pool before clearing cache when sorting fix: make explicit data request when clearing cache with no initial pool Jan 18, 2021
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Just a few comments related to the code style.

test/data-provider.test.js Outdated Show resolved Hide resolved
test/data-provider.test.js Outdated Show resolved Hide resolved
test/data-provider.test.js Outdated Show resolved Hide resolved
test/data-provider.test.js Outdated Show resolved Hide resolved
yuriy-fix and others added 2 commits January 18, 2021 11:17
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
@yuriy-fix yuriy-fix merged commit 22ee36b into master Jan 18, 2021
@yuriy-fix yuriy-fix deleted the fix-hidden-sorting branch January 18, 2021 10:18
yuriy-fix added a commit that referenced this pull request Jan 25, 2021
…ool (#2119)

* fix: increase pool before clearing cache when sorting; add test

* Enhance the fix by making explicit data request

* Remove height condition

* Move the test

* Apply Serhii's suggestions

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>

* Update test

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
yuriy-fix added a commit that referenced this pull request Feb 1, 2021
…ool (#2119) (#2123)

* fix: increase pool before clearing cache when sorting; add test

* Enhance the fix by making explicit data request

* Remove height condition

* Move the test

* Apply Serhii's suggestions

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>

* Update test

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>

Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot sort a Grid that is initially hidden
4 participants