fix: modify the BatchHttpLink to have each timer when the batch key is different #10408
Conversation
|
@zlrlo: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
🦋 Changeset detectedLatest commit: ea7ec33 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
zlrlo
force-pushed
the
fix-scheduleQueueConsumption
branch
2 times, most recently
from
34a0f6a
to
c8b92fa
Compare
|
Hi @zlrlo 👋🏻 thanks for opening this PR! It looks like there are some failing tests, mind taking a closer look at those? Feel free to ask questions in this PR 🙏🏻 |
zlrlo
force-pushed
the
fix-scheduleQueueConsumption
branch
from
c8b92fa
to
f583cc8
Compare
src/link/batch/batching.ts
Outdated
| clearTimeout(this.scheduledBatchTimerByKey.get(key)); | ||
| this.scheduledBatchTimerByKey.set(key, setTimeout(() => { | ||
| this.consumeQueue(key); | ||
| }, this.batchInterval); | ||
| }, this.batchInterval)); |
There was a problem hiding this comment.
There doesn't seem to be a part that deletes the timer in scheduledBatchTimersByKey.
There was a problem hiding this comment.
after consume, don't we remove timer from map?
just like this.scheduledBatchTimerByKey.delete(key)
bignimbus
force-pushed
the
fix-scheduleQueueConsumption
branch
from
e9e0d49
to
7cb861f
Compare
There was a problem hiding this comment.
Thanks again @zlrlo for submitting this PR. I rebased from main and pushed @kang-heewon's suggestion. I also added an inline comment to explain that timeout = real failure in the test case @zlrlo added. We don't have a runnable reproduction handy so if possible please verify that the latest version of this branch works for you on your end. I think we're close to being able to merge this 🙏🏻
| this.consumeQueue(key); | ||
| }, this.batchInterval); | ||
| this.scheduledBatchTimerByKey.delete(key); |
There was a problem hiding this comment.
Shouldn't this delete be done once more right after clearTimeout?
It is not deleted from the map when clearTimeout runs. @bignimbus
There was a problem hiding this comment.
Hi @kang-heewon - I felt that would be redundant given that we're setting a new value for that key on line 215. Can you help me understand your suggestion more?
There was a problem hiding this comment.
Oh, you're right. It's my mistake.
There was a problem hiding this comment.
I was able to validate this fix on this reproduction scenario, forked from @vpotapchuk's example here: #10205 (comment)
bignimbus
force-pushed
the
fix-scheduleQueueConsumption
branch
from
8fb511e
to
12668b0
Compare
bignimbus
removed
the
🏓 awaiting-team-response
|
Hi @bignimbus The latest version of this branch is working well for us. Thank you. |
fix #10205
In version 3.7.3, I found that the mutation not issued on network requests in useEffect or onCompleted.
If there is a query request after the mutation request, the query cleared the timer for the mutation.
So I made a timer by separating it with a batch key and this problem was fixed.
Checklist: