Closed lucasyvas closed 3 years ago
I have a similar issue with loading APIs which are fast, and I don't want to show loading until 1000 ms.
I was worked a project on GitHub which resolved this problem. https://github.com/hosseinmd/react-concurrent I want to migrate on react-query because I don't have enough time to documenting my own repo to my team. Another reason is the community of this project, Of course, I think the source code of this project seems very complex.
Does this resolve your issue?
import { notifyManager } from "react-query"
notifyManager.setBatchNotifyFunction(callback => {
callback()
})
Do you think something is wrong with the implementation in react query or would this happen in any ReactDOM.unstable_batchedUpdates
implementation?
Does this resolve your issue?
import { notifyManager } from "react-query" notifyManager.setBatchNotifyFunction(callback => { callback() })
Do you think something is wrong with the implementation in react query or would this happen in any
ReactDOM.unstable_batchedUpdates
implementation?
Thank you for the suggestion - I should have double checked to see if any suitable functionality was exposed on the API! I will report back with requested findings/comments when I get a chance (within a couple of days).
Upon further investigation, it would at least appear that the call to batching is fine, but how the batch gets scheduled is surfacing the issue. By monkey-patching the flush
call to not apply the batch after the next tick, the issues I'm having completely disappear:
import { notifyManager } from 'react-query';
notifyManager.flush = function flush() {
const queue = this.queue;
this.queue = [];
if (queue.length) {
this.batchNotifyFn(() => {
queue.forEach((callback) => {
this.notifyFn(callback);
});
});
}
}.bind(notifyManager);
The original flush method (delaying batch until next tick):
flush(): void {
const queue = this.queue
this.queue = []
if (queue.length) {
scheduleMicrotask(() => {
this.batchNotifyFn(() => {
queue.forEach(callback => {
this.notifyFn(callback)
})
})
})
}
}
I'm afraid I don't have enough knowledge of the ReactDOM.unstable_batchedUpdates
API to know what the best course of action would be in terms of altering the existing implementation. For what it's worth, I do have a couple of comments:
react-query
) take control over rendering in such a manner is extremely surprisingI would love to provide a sample, but it may be an onerous reproduction - we are encountering this in our product development and I'd have to see what the likelihood of providing some type of reproduction link would be.
Would love to get a playground to debug the issue. The timeout is actually to make sure RQ does not trigger React state updates while React is rendering (it'll trigger an error otherwise). Maybe there are other ways to handle it. Regarding your comment: RQ is not just a fetching lib because it also provides a view integration for React (which we try to optimize as much as possible). The same batching technique is also used in Redux for example.
Until we can get a more solid reproduction case of this, it sounds like we're doing our best with regards to the ecosystem standards. Open to discussing this more if and when more info comes forward.
We're having issues with the batching/scheduler too. I don't have a sandbox/public repo I can share (yet, maybe).
Using @lucasyvas 's flush monkeypatch seems to cut down the microtask portion of the profiler, but the batcher is still taking up quite a bit of time.
This is only with one active query changing, which is being managed and changed via a page
prop change in a React context.
@dylangarcia Just replying to say that I unfortunately didn't get anywhere on a good reproduction to offer the team - it was difficult to find time to get to the simplest reproduction.
The only conclusion I reached was the one I did prior, and it's that I wish I - as a consuming developer - was given control over this since what is there doesn't seem to function ideally in some cases.
I was able to alter our experience a bit to make it less noticeable, but that's about it.
Describe the bug
Critical UI updates derived from queries can be batched into later cycles, creating a detrimental experience. We have a fairly large component that shows loading spinners derived from
isLoading
. We are using a mix ofuseQuery
anduseQueries
, and unfortunately the internal batching strategy can lead to more critical visual updates being delayed. In this case, fewer re-renders is not good, as they occur near the tail end.To Reproduce
There may be several variables, but here is our scenario:
isLoading
statusExpected behavior
I expect to be able to control data/renders in my own hooks with refs or state, and not always use the built-in batching behaviour.
Desktop (please complete the following information):
Additional context
As a quick, very naive hack, I replaced the
scheduleMicrotask
function here:https://github.com/tannerlinsley/react-query/blob/80cecef22c3e088d6cd9f8fbc5cd9e2c0aab962f/src/core/utils.ts#L362
with the following:
This substantially improved UI responsiveness in our case, dramatically improving timings by removing the batch update triggered from here:
https://github.com/tannerlinsley/react-query/blob/ac342e237ae9fab6759f2d7be616662da6c16225/src/core/notifyManager.ts#L68
I would like to request that batching either be opt-in or opt-out, either globally or maybe per query. I'd be perfectly happy with a global workaround if it provides a quicker resolution.
Edit: Forgot to mention I may be able to help with this depending on what the best course of action is!