Queries throw away data when skipped

[markerikson]

Seems like the changes in #2779 broke some existing behavior:

• v1.9.0 #2865 (comment)
• RTK-Query: add ability to get access to latest batch info when using merge #2870 (comment)

[phryneas]

Yeah. It seems that before, the behavior was a bit unpredictable - sometimes going one way, sometimes another. And #2779 fixed it towards the tested behavior that didn't always apply.

We should probably add a { skip: 'keep' }/skipAndKeepToken to have the behavior always predictable.

[ivan-jelev]

I would not expect to have {skip: 'keep'} or skipAndKeepToken.. it adds unneeded verbosity and confusion.. Instead {skip: true} should only prevent new requests and should not impact data / currentData for queries which already has some cached data.

Well.. maybe it should impact only currentData though

[phryneas]

@ivan-jelev Well, we had tests in place since the initial release of RTK Query that made sure that skip: true should not return any data.

Honestly, I was unaware that it did return data in some circumstances. Still, now we are in a situation where we know that sometimes it returned data and sometimes it didn't - apparently based on some internal timing.

The best way going forwards seems to ensure that the actual behavior always is the behavior we have always been testing for and that was also the intended behavior - and adding a new behavior for those that want the "sometimes accidental" behavior.

[ivan-jelev]

Right.. it makes perfect sense for granular/atomic queries. However now there's more cases and with merge we actually have ability to see not just granular query state, but accumulated state of query series. {skip: true} in context of granular query is expected to behave as designed initially, but in case of accumulated state it's not expected to loose all data because of one skip in a seria.

An option might be to make skip behaviour different for queries with merge and without. Or to introduce suggested { skip: 'keep' } / skipAndKeepToken.. but in any case we need a way to prevent data / currentData reset.

UPD: 2d option now looks better for me - { skip: 'keep' } / skipAndKeepToken. Backward compatible & fully controllable behaviour

[piotrlubecki]

I think that current behaviour is inconsistent between useQuery and useLazyQuery. In useLazyQuery data always returns latest returned result regardless of trigger arg and currentData considers args. useQuery behaviour according to documentation should be similar (https://redux-toolkit.js.org/rtk-query/api/created-api/hooks#usequery), however when skip: true data is undefined.

When it is necessary to get latest returned result regardless of trigger args and sometimes skip request, now it is necessary to use useLazyQuery. useLazyQuery works fine, but it requires more code than useQuery (It is necessary to write logic already provided in useQuery).

I personally find it more intuitive when data always returned latest result. I am happy with achieving it using { skip: 'keep' } / skipAndKeepToken, but maybe this should be the default behaviour and returning undefined should be an option (then useQuery and useLazyQuery will have consistent behaviour)?

[phryneas]

@piotrlubecki You can't skip a lazy query, so how could the skip behavior between a hook that has that functionality and a hook that never knows that concept ever be consistent?

[piotrlubecki]

@phryneas Yes, there is no skip, but it can be triggered conditionally, which behaves as skip.
According to documentation useQuery and useLazyQuery should be similar, so maybe if useLazyQuery always returns latest result in data then useQuery should do the same, even is skip is used (as skip seems like best equivalent of triggering useLazyQuery conditionally)?

[phryneas]

@piotrlubecki

They behave exactly the same:

1. You call useQuery(skipToken) - you get no result
2. You call useLazyQuery() - you get no result
3. You call useQuery(something) you get a result
4. You call trigger you get a result

Those are the four cases both have common. There is one additional case:
5. You call useQuery(skipToken) after useQuery(something)

That additional case is not possible with useLazyQuery. It is useQuery only, and it behaves just exactly like 1. - because it is the same thing.

PS: there is even an equivalent to 5. - you can call .reset on the lazy query, which will again bring you back to 2.
I don't see any inconsistency here?

[piotrlubecki]

@phryneas I am thinking about this scenario:

const [trigger, result] = useLazyQuery();

if (condition){
   trigger(args);
}

I thought that above could be compared to this:

const { data } = useQuery(args, { skip: !condition } );

In this scenario, assuming that point 3 and 4 from your explanation were called at least once, result.data != data when condition is false. useLazyQuery result.data will always return latest result, after it will be called at least once. useQuery data will return undefined, if condition is false, even if it was called before.

I might been wrong to use skip this way, as it was unintended behaviour, but it has worked really nice for me and it was simpler than using useLazyQuery.

[phryneas]

Well, in your example, the trigger sets the argument and then keeps it forever. There is no skip behavior there.

I want to step back a bit and want to ask: why do you even want to use skip if you deliberately want to access that data?

[piotrlubecki]

I am building online store, and simplifying the problem user is proving products, quantity and discount coupons. This values are used by backend to calculate price and return which products and coupons can be in cart (if quantity do not exceed quantity in stock and if coupons are applicable for this order).

Response from server is sometimes modify original args (as quantity can be decreased or coupon can be incorrect). If backend response has modified any of the args, I used skip to deliberately do not make another request to backend, as I was sure that it will return the same results. Thanks to skip I have used only one state of products, quantity and coupons.

Since redux-toolkit 1.9.0 I rewritten the logic to useLazyQuery, instead of useQuery which I have used in previous versions.

BTW. @phryneas thank you for taking your time to discuss this issue and maintaining this great product, which I use at work and in hobby projects.

[defaktor]

As we see in documentation: https://redux-toolkit.js.org/rtk-query/usage/conditional-fetching
"If skip: false is set after the initial load, the cached result will be used" - and this is good way to use skip option.

[phryneas]

As we see in documentation: https://redux-toolkit.js.org/rtk-query/usage/conditional-fetching
"If skip: false is set after the initial load, the cached result will be used" - and this is good way to use skip option.

That is a very good point - now we do have a problem 🤔 if this was actually documented, we might need to roll it back.
Thank you very much for pointing that out!

[jarosik10]

@phryneas What about the rollback?

[rubimbura]

@phryneas is there a problem if I use mutation instead of Query to call a GET end-point? with muation I dont have to deal with skip.

[markerikson]

@rubimbura : "queries" and "mutations" aren't about GET vs POST. A query is "fetching and caching data from the server", and a query is about "updating data on the server".

You can have a query that's a POST request. You could, occasionally, have a mutation that's a GET request.

[thesteady]

This has been a breaking change for my app, so wanted to add some notes in case its helpful in decision-making here. We were relying on a previously run query that then gets skipped to keep its data in several places across our application. Upgrading from 1.7.2 to 1.9.2 caused us to suddenly lose data on skipped queries after it was successfully fetched.

We use a combination of skip and pollingInterval, along with a React useState variable to poll an endpoint for a status on a long-running job. When the query response indicates that the job is complete, we don't need further updates and so we were setting React state to true / complete, which in turn causes skip to become true. Example:

const [jobIsComplete, setComplete] = React.useState(false);
const myQuery = useCoolQuery(queryArgs, { 
   skip: missingQueryArg || jobIsComplete,
   pollingInterval: someTime
});

Before, this worked because we were still getting data on skipped queries. Upgrading, we would suddenly lose data after a successful query. For most cases, it was relatively easy to instead set the pollingInterval to 0 when the state updated to true. That feels like the better solution to me for these queries in general, but took a bit of digging to figure out what was happening. For one other query, we've had to add this pollingInterval check as a bit of hack to avoid losing data on a re-render.

A note somewhere (release notes, docs) that behavior has changed would be super helpful for faster troubleshooting, I'd imagine.

p.s. thank you for your hard work!

[markerikson]

@thesteady : for the record this is a "bug" and not an intended breaking change.

We do need to fix this, but tbh it's kind of slipped off the radar due to the holidays, personal life, and other priorities.

[thesteady]

Cool thank you! I read through the comments and wasn't clear, my bad.

[rubimbura]

Many thanks @markerikson.

[markerikson]

Could folks try out the CodeSandbox preview build in #3188 and let me know if that fixes the skipping issue?

[kasparkallas]

Nooooooooo! Absolutely preferred the new behaviour... :(

Codebase is riddled with this type of usages:

useAccountQuery(account ? { account: account.address } : skipToken); // Web3 use-case

useSelectionDataQuery(selection ? { id: selection.id } : skipToken); // Form use-case

[markerikson]

@kasparkallas : then use const { currentData } = useQuery() instead, which reflects "the value for the current args".