fix(ktabledata): loading prop should override internal loading state
#2614
Conversation
Justineo
requested review from
kongponents-bot,
adamdehaven,
jillztom,
kaiarrowood and
a team
as code owners
✅ Deploy Preview for kongponents-sandbox ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kongponents ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Preview package from this PR in consuming applicationIn consuming application project install preview version of kongponents generated by this PR: |
| @@ -19,7 +19,7 @@ | |||
| :hide-pagination="hidePagination || !showPagination" | |||
| :hide-pagination-when-optional="false" | |||
| :hide-toolbar="hideToolbar" | |||
| :loading="loading || fetcherIsLoading" | |||
| :loading="loading ?? fetcherIsLoading" | |||
There was a problem hiding this comment.
It appears that it’s common practice across our tables in Konnect to initialize a ref<boolean> that defaults to false for many of our KTableData instances.
Wouldn’t this usage pattern prevent the table’s built-in loading state from showing when it actually does need to initially fetch the data when a component mounts?
There was a problem hiding this comment.
In the linked implementation, each time fetcher is called, it triggers clearLoadingAndError, which sets isLoading to true. The logic seems a bit convoluted, but I assume the behavior remains consistent. We can confirm this by testing it in a preview PR.
Actually, I believe we’re misusing fetcherCacheKey to trigger revalidation like this. Instead, we should directly include the fetcher parameters in the cache key.
For the current change, if users explicitly set the loading prop to false, we should consider respecting that.
There was a problem hiding this comment.
The behavior of the loading prop isn’t the core of the problem we’re facing. The key issue is that we haven’t exposed a “revalidate only” interface on the Table. The auto-incrementing cache key actually implies loading new data that hasn’t been loaded before, which, technically, is not accurate. Another viable solution is to defineExpose the revalidate method from <KTableData>, and also, for the scenario we’re currently encountering (polling), expose the refreshInterval configuration of SWRV as a prop. This way, we can trigger polling revalidation in a more declarative manner. WDYT?
Summary
KM-952
Currently, our products usually follow the approach described in the Kongponents documentation, where revalidation is triggered by incrementing the
fetcherCacheKey(although strictly speaking, this is not the correct method, as it actually caches data for the same query at different locations, which is loading new data instead of revalidation).After adding the
isLoadingstate for SWRV,<KTableData>also triggers the loading state when thefetcherCacheKeychanges (which is technically correct, but the semantics of incrementing thefetcherCacheKeyare slightly different). This is generally acceptable (for example, after adding or removing list items).However, in certain scenarios like polling, we actually don’t want to trigger the loading state. But since we haven’t exposed the revalidate-related primitives, users can only achieve this by incrementing the
fetcherCacheKey, which may lead to unexpected loading states. A quick workaround is for users to control theloadingprop themselves to prevent the loading state during polling.This PR implements a solution where, if the user passes the
loadingprop, it will always override the internal loading state of the component (which is also the expected behavior for theloadingprop), helping users with a quick workaround.Also updated Cypress to fix cypress-io/cypress#30715.