add `currentData` property to hook results. #1500

phryneas · 2021-09-12T08:19:16Z

This would be one possible solution for #1339.

Feedback very welcome.

TODO:

Tests
Docs

netlify · 2021-09-12T08:23:18Z

✔️ Deploy Preview for redux-starter-kit-docs ready!

🔨 Explore the source changes: efbf1cf

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/613db80528a9c00007a3cd35

😎 Browse the preview: https://deploy-preview-1500--redux-starter-kit-docs.netlify.app

codesandbox-ci · 2021-09-12T08:23:25Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit efbf1cf:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
rsk-github-issues-example	Configuration
@examples-query-react/basic	Configuration
@examples-query-react/advanced	Configuration

github-actions · 2021-09-12T08:24:21Z

size-limit report 📦

Path	Size
1. entry point: @reduxjs/toolkit (cjs.production.min.js)	12.23 KB (0%)
1. entry point: @reduxjs/toolkit (esm.js)	10.22 KB (0%)
1. entry point: @reduxjs/toolkit/query (cjs.production.min.js)	20.34 KB (0%)
1. entry point: @reduxjs/toolkit/query (esm.js)	17.37 KB (0%)
1. entry point: @reduxjs/toolkit/query/react (cjs.production.min.js)	22.22 KB (+0.03% 🔺)
1. entry point: @reduxjs/toolkit/query/react (esm.js)	19.82 KB (+0.03% 🔺)
2. entry point: @reduxjs/toolkit (without dependencies) (cjs.production.min.js)	5.55 KB (0%)
2. entry point: @reduxjs/toolkit (without dependencies) (esm.js)	5.53 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (cjs.production.min.js)	9.2 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (esm.js)	9.58 KB (0%)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (cjs.production.min.js)	2.37 KB (+0.29% 🔺)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (esm.js)	2.21 KB (+0.31% 🔺)
3. createSlice (esm.js)	5.16 KB (0%)
3. createEntityAdapter (esm.js)	5.83 KB (0%)
3. configureStore (esm.js)	5.83 KB (0%)
3. createApi (esm.js)	15.66 KB (0%)
3. createApi (react) (esm.js)	18.05 KB (+0.04% 🔺)
3. fetchBaseQuery (esm.js)	10.93 KB (0%)
3. setupListeners (esm.js)	9.8 KB (0%)
3. ApiProvider (esm.js)	16.99 KB (+0.04% 🔺)

HansBrende · 2021-09-12T17:03:09Z

packages/toolkit/src/query/react/buildHooks.ts

@@ -420,6 +436,7 @@ const queryStatePreSelector = (
  return {
    ...currentState,
    data,
+    currentData: currentState.data,


Correct me if I am wrong, but wouldn't this be basically the equivalent of doing:

const currentData = isFetching ? undefined : data;

in our own code? I would think you'd need to examine the args associated with lastResult to see if they are shallowly equal to the args of currentState, something like:

currentData = currentState.data ?? (lastResult?.arg === currentState.arg ? lastResult?.data : undefined)

if the comment

this property will always contain the received data from the query, for the current query arguments

is to be true. (i.e., distinguishing cases of polling from cases of arg changes).

But, correct me if I am wrong here. I am not too familiar with the internal workings of this codebase.

currentState is always for the currently selected endpoint and lastResult is the last result the hook had cached. So lastResult can also be from another arg, but currentState never can.

@phryneas Right... but my question is: this solution doesn't seem to distinguish between polling and arg changes.
The behavior I'd expect for currentData is that if the args haven't changed (e.g., because of polling), then it would use the last result for those args (which might be cached in lastResult in that case, if a refetch is currently taking place).

Data once present for the current args will not disappear from the cache while polling, and this essentially gives you data for the current args directly from the cache.
Once data is set in the cache, it will never be removed unless there is new data to replace it with, even while loading or an error occurs.
See the slice definition here:

redux-toolkit/packages/toolkit/src/query/core/buildSlice.ts

Lines 121 to 169 in 261410a

.addCase(queryThunk.pending, (draft, { meta, meta: { arg } }) => {

if (arg.subscribe) {

// only initialize substate if we want to subscribe to it

draft[arg.queryCacheKey] ??= {

status: QueryStatus.uninitialized,

endpointName: arg.endpointName,

}

}

updateQuerySubstateIfExists(draft, arg.queryCacheKey, (substate) => {

substate.status = QueryStatus.pending

substate.requestId = meta.requestId

substate.originalArgs = arg.originalArgs

substate.startedTimeStamp = meta.startedTimeStamp

})

})

.addCase(queryThunk.fulfilled, (draft, { meta, payload }) => {

updateQuerySubstateIfExists(

draft,

meta.arg.queryCacheKey,

(substate) => {

if (substate.requestId !== meta.requestId) return

substate.status = QueryStatus.fulfilled

substate.data = copyWithStructuralSharing(substate.data, payload)

delete substate.error

substate.fulfilledTimeStamp = meta.fulfilledTimeStamp

}

)

})

.addCase(

queryThunk.rejected,

(draft, { meta: { condition, arg, requestId }, error, payload }) => {

updateQuerySubstateIfExists(

draft,

arg.queryCacheKey,

(substate) => {

if (condition) {

// request was aborted due to condition (another query already running)

} else {

// request failed

if (substate.requestId !== requestId) return

substate.status = QueryStatus.rejected

substate.error = (payload ?? error) as any

}

}

)

}

)

},

agusterodin · 2021-09-12T17:23:34Z

I wish I had something more insightful to add (based on my limited knowledge of the inner workings of how this library works) but for what it is worth:

currentData behaves exactly as advertised in the comment that shows up in intellisense. It solves the issue I was personally having. Just tried it out in my code and can confirm.

In general I really like this solution compared to other approaches like adding isLoadingFirstTimeForArg or having a keepPreviousData option in the hook (like React-Query does).

HansBrende · 2021-09-12T17:33:10Z

@agusterodin out of curiosity, did you also try it out for polling situations? I would expect that currentData doesn't get reset back to undefined while a polling operation is taking place (since the args haven't changed).

phryneas · 2021-09-12T17:51:40Z

@agusterodin out of curiosity, did you also try it out for polling situations? I would expect that currentData doesn't get reset back to undefined while a polling operation is taking place (since the args haven't changed).

It doesn't get reset, because also the cache entry does not disappear. But you now have the direct distinction:
isSuccess && !isFetching => fresh data in currentData. currentData available otherwise -> might just be from an older request for these args.

HansBrende · 2021-09-12T18:06:11Z

@phryneas great! If currentData doesn't get reset back to undefined if the args don't change, then that's exactly what I'm looking for.

One other question: I'd expect that currentData gets set to undefined whenever the skipToken is passed in as the current args... will that be the case in this implementation?

agusterodin · 2021-09-12T18:06:36Z

Just went back and checked, can confirm that currentData remains stable even when polling is enabled (which is desirable behavior) 👍

phryneas · 2021-09-12T18:45:38Z

One other question: I'd expect that currentData gets set to undefined whenever the skipToken is passed in as the current args... will that be the case in this implementation?

From the back of my head, yes - but please validate.

msutkowski

This makes sense to me - @Shrugsy perhaps we can add meaningful examples to the docs based on the use cases from #1339 (comment) once/if this lands?

HansBrende · 2021-09-12T19:29:17Z

@phryneas I've just now installed and tested it: currentData does maintain the expected behavior with skipToken. Nice work! 🎉

Only other recommendation I can think of: I expect most devs will end up migrating from data to currentData as soon as they discover the potential integrity problems with data (which are not immediately obvious until you realize that your UI is behaving weirdly and have to dig down to the root cause). In fact, I can see currentData as becoming sort of the default recommended approach to take, whereas "data" would then only be used in edge cases such as paginated queries, etc. where you know that you really do want that behavior. That being the case, it might be prudent to name it something slightly more succinct and "default-sounding", such as, simply: result.

All-in-all, great work on this! It definitely fixes the UI bugs I've been having. Hope it can be released soon :)

phryneas · 2021-09-12T19:43:36Z

Only other recommendation I can think of: I expect most devs will end up migrating from data to currentData as soon as they discover the potential integrity problems with data (which are not immediately obvious until you realize that your UI is behaving weirdly and have to dig down to the root cause). In fact, I can see currentData as becoming sort of the default recommended approach to take, whereas "data" would then only be used in edge cases such as paginated queries, etc. where you know that you really do want that behavior. That being the case, it might be prudent to name it something slightly more succinct and "default-sounding", such as, simply: result.

Honestly, I think it is niche and will probably also stay niche - the current behaviour is not by accident, but very much intended. I can see where it makes sense to use currentData in polling situations, but for most simpler applications I think data more closely models what people actually want - at least until we get suspense for data fetching and then we'll probably have to ship a second set of hooks or something and that whole question becomes irrelevant.

HansBrende · 2021-09-12T19:47:23Z

@phryneas sounds good; I disagree with the "niche" part, but whatever you name it, I will use it! Again, nice work on this. Will be looking forward to the next release 😄

Shrugsy · 2021-09-13T09:40:47Z

This makes sense to me - @Shrugsy perhaps we can add meaningful examples to the docs based on the use cases from #1339 (comment) once/if this lands?

yep, can do

saschametz · 2021-09-20T07:54:38Z

The currentData addition is exactly what we need for our migration from react-query 👍

In our case we need the currentData property most of the time and the data property only for some parts of our application. The only thing that was confusing at first was that the isLoading property is not set to false for subsequent queries. To get the desired loading behavior, we simply use currentData === undefined.

For example, we want to use currentData for a paginated query and display a loading indicator if the current data has not yet been fetched:

Loading the first page:

data = undefined, currentData = undefined => isLoading = true, isFetching = true
data = ["foobar 1"], currentData = ["foobar 1"] => isLoading = false, isFetching = false

Loading the second page:

data = ["foobar 1"], currentData = undefined => isLoading = false, isFetching = true
data = ["foobar 2"], currentData = ["foobar 2"] => isLoading = false, isFetching = false

Note: The other option isFetching wouldn't solve this problem, because we don't want to display a loading indicator when there is already data in the cache.

react-query doesn't has this "problem" that isLoading is false in step 3 because they only have one data prop. Whether the data prop contains previous data or not depends on an additional option keepPreviousData: true.

Example:
https://codesandbox.io/s/examples-query-react-basic-forked-wiezj

Example 2.):
While the first loading indicator is displayed, the loading indicator for the second page is missing due to the described behavior.

add currentData property to hook results.

efbf1cf

phryneas mentioned this pull request Sep 12, 2021

RTKQ: Data stays as old value and loading immediately jumps back to false when fetching fresh data from different endpoint + query param combination #1339

Closed

HansBrende reviewed Sep 12, 2021

View reviewed changes

msutkowski approved these changes Sep 12, 2021

View reviewed changes

justindomingue mentioned this pull request Sep 13, 2021

feat: routing api integration Uniswap/interface#2116

Merged

phryneas added this to the 1.7 milestone Sep 15, 2021

msutkowski self-assigned this Sep 15, 2021

phryneas changed the base branch from master to v1.7.0-integration October 1, 2021 08:18

phryneas changed the title ~~[RFC] add currentData property to hook results.~~ add currentData property to hook results. Oct 1, 2021

phryneas merged commit 18ef51d into v1.7.0-integration Oct 1, 2021

phryneas deleted the pr/hook-currentData branch October 1, 2021 08:19

phryneas mentioned this pull request Oct 1, 2021

v1.7.0 integration branch #1565

Merged

Shrugsy mentioned this pull request Oct 5, 2021

📝 Document currentData in query hook results #1576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `currentData` property to hook results. #1500

add `currentData` property to hook results. #1500

phryneas commented Sep 12, 2021 •

edited by msutkowski

Loading

netlify bot commented Sep 12, 2021

codesandbox-ci bot commented Sep 12, 2021

github-actions bot commented Sep 12, 2021

HansBrende Sep 12, 2021 •

edited

Loading

phryneas Sep 12, 2021

HansBrende Sep 12, 2021 •

edited

Loading

phryneas Sep 12, 2021 •

edited

Loading

agusterodin commented Sep 12, 2021

HansBrende commented Sep 12, 2021

phryneas commented Sep 12, 2021 •

edited

Loading

HansBrende commented Sep 12, 2021

agusterodin commented Sep 12, 2021

phryneas commented Sep 12, 2021

msutkowski left a comment

HansBrende commented Sep 12, 2021

phryneas commented Sep 12, 2021

HansBrende commented Sep 12, 2021

Shrugsy commented Sep 13, 2021

saschametz commented Sep 20, 2021 •

edited

Loading

	.addCase(queryThunk.pending, (draft, { meta, meta: { arg } }) => {
	if (arg.subscribe) {
	// only initialize substate if we want to subscribe to it
	draft[arg.queryCacheKey] ??= {
	status: QueryStatus.uninitialized,
	endpointName: arg.endpointName,
	}
	}

	updateQuerySubstateIfExists(draft, arg.queryCacheKey, (substate) => {
	substate.status = QueryStatus.pending
	substate.requestId = meta.requestId
	substate.originalArgs = arg.originalArgs
	substate.startedTimeStamp = meta.startedTimeStamp
	})
	})
	.addCase(queryThunk.fulfilled, (draft, { meta, payload }) => {
	updateQuerySubstateIfExists(
	draft,
	meta.arg.queryCacheKey,
	(substate) => {
	if (substate.requestId !== meta.requestId) return
	substate.status = QueryStatus.fulfilled
	substate.data = copyWithStructuralSharing(substate.data, payload)
	delete substate.error
	substate.fulfilledTimeStamp = meta.fulfilledTimeStamp
	}
	)
	})
	.addCase(
	queryThunk.rejected,
	(draft, { meta: { condition, arg, requestId }, error, payload }) => {
	updateQuerySubstateIfExists(
	draft,
	arg.queryCacheKey,
	(substate) => {
	if (condition) {
	// request was aborted due to condition (another query already running)
	} else {
	// request failed
	if (substate.requestId !== requestId) return
	substate.status = QueryStatus.rejected
	substate.error = (payload ?? error) as any
	}
	}
	)
	}
	)
	},

add currentData property to hook results. #1500

add currentData property to hook results. #1500

Conversation

phryneas commented Sep 12, 2021 • edited by msutkowski Loading

TODO:

netlify bot commented Sep 12, 2021

codesandbox-ci bot commented Sep 12, 2021

github-actions bot commented Sep 12, 2021

size-limit report 📦

HansBrende Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

phryneas Sep 12, 2021

Choose a reason for hiding this comment

HansBrende Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

phryneas Sep 12, 2021 • edited Loading

Choose a reason for hiding this comment

agusterodin commented Sep 12, 2021

HansBrende commented Sep 12, 2021

phryneas commented Sep 12, 2021 • edited Loading

HansBrende commented Sep 12, 2021

agusterodin commented Sep 12, 2021

phryneas commented Sep 12, 2021

msutkowski left a comment

Choose a reason for hiding this comment

HansBrende commented Sep 12, 2021

phryneas commented Sep 12, 2021

HansBrende commented Sep 12, 2021

Shrugsy commented Sep 13, 2021

saschametz commented Sep 20, 2021 • edited Loading

add `currentData` property to hook results. #1500

add `currentData` property to hook results. #1500

phryneas commented Sep 12, 2021 •

edited by msutkowski

Loading

HansBrende Sep 12, 2021 •

edited

Loading

HansBrende Sep 12, 2021 •

edited

Loading

phryneas Sep 12, 2021 •

edited

Loading

phryneas commented Sep 12, 2021 •

edited

Loading

saschametz commented Sep 20, 2021 •

edited

Loading