-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat(query-core): add timeoutManager to allow changing setTimeout/setInterval #9612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 7 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
8fc4d3d
add timeoutManager class
justjake 2fb1f1f
add additional types & export functions
justjake acb595c
convert all setTimeout/setInterval to managed versions
justjake 96813b6
tweaks
justjake 530504e
add claude-generated tests
justjake 223371b
tests
justjake d3d7d1a
revert changes in query-async-storage-persister: no path to import qu…
justjake a953c70
re-export more types
justjake 0fac7b2
console.warn -> non-production console.error
justjake 47962fd
query-async-storage-persister: use query-core managedSetTimeout
justjake 62b1dd0
pdate pnpm-lock for new dependency edge
justjake 8d5e050
sleep: always managedSetTimeout
justjake fea6cce
remove managed* functions, call method directly
justjake 1606b58
remove runtime coercion and accept unsafe any within TimeoutManager c…
justjake fc9092b
cleanup; fix test after changes
justjake 5d6fe4d
name is __TEST_ONLY__
justjake ad1fb2b
notifyManager: default scheduler === systemSetTimeoutZero
justjake 932c3a2
Improve TimeoutCallback comment since ai was confused
justjake a6d38f8
remove unnecessary timeoutManager-related exports
justjake 81d35ac
prettier-ify index.ts (seems my editor messed with it already this pr?)
justjake b6fffb4
continue to export defaultTimeoutProvider for tests
justjake 948c646
oops missing import
justjake 08a2c5f
Merge branch 'main' into jake--timeoutmanager
TkDodo 841ac54
fix: export systemSetTimeoutZero from core
TkDodo fb22c67
ref: use notifyManager.schedule in createPersister
TkDodo 09a787e
ref: move provider check behind env check
TkDodo 7fb57b1
docs
justjake 4b1e8af
doc tweaks
justjake b6ca80d
doc tweaks
justjake 73014f5
docs: reference timeoutManager in discussion of 24 day setTimout limit
justjake 3f452ea
Apply suggestion from @TkDodo
TkDodo cca861f
Apply suggestion from @TkDodo
TkDodo d58e28f
chore: fix broken links
TkDodo 7922966
docs: syntax fix
TkDodo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
204 changes: 204 additions & 0 deletions
204
packages/query-core/src/__tests__/timeoutManager.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,204 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { | ||
| TimeoutManager, | ||
| defaultTimeoutProvider, | ||
| managedClearInterval, | ||
| managedClearTimeout, | ||
| managedSetInterval, | ||
| managedSetTimeout, | ||
| systemSetTimeoutZero, | ||
| timeoutManager, | ||
| } from '../timeoutManager' | ||
|
|
||
| describe('timeoutManager', () => { | ||
| function createMockProvider(name: string = 'custom') { | ||
| return { | ||
| name, | ||
| setTimeout: vi.fn(() => 123), | ||
| clearTimeout: vi.fn(), | ||
| setInterval: vi.fn(() => 456), | ||
| clearInterval: vi.fn(), | ||
| } | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.spyOn(console, 'warn') | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| }) | ||
|
|
||
| describe('TimeoutManager', () => { | ||
| let manager: TimeoutManager | ||
|
|
||
| beforeEach(() => { | ||
| manager = new TimeoutManager() | ||
| }) | ||
|
|
||
| it('by default proxies calls to globalThis setTimeout/clearTimeout', () => { | ||
| const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') | ||
| const clearTimeoutSpy = vi.spyOn(globalThis, 'clearTimeout') | ||
| const setIntervalSpy = vi.spyOn(globalThis, 'setInterval') | ||
| const clearIntervalSpy = vi.spyOn(globalThis, 'clearInterval') | ||
|
|
||
| const callback = vi.fn() | ||
| const timeoutId = manager.setTimeout(callback, 100) | ||
| expect(setTimeoutSpy).toHaveBeenCalledWith(callback, 100) | ||
| clearTimeout(timeoutId) | ||
|
|
||
| manager.clearTimeout(200) | ||
| expect(clearTimeoutSpy).toHaveBeenCalledWith(200) | ||
|
|
||
| const intervalId = manager.setInterval(callback, 300) | ||
| expect(setIntervalSpy).toHaveBeenCalledWith(callback, 300) | ||
| clearInterval(intervalId) | ||
|
|
||
| manager.clearInterval(400) | ||
| expect(clearIntervalSpy).toHaveBeenCalledWith(400) | ||
| }) | ||
|
|
||
| describe('setTimeoutProvider', () => { | ||
| it('proxies calls to the configured timeout provider', () => { | ||
| const customProvider = createMockProvider() | ||
| manager.setTimeoutProvider(customProvider) | ||
|
|
||
| const callback = vi.fn() | ||
|
|
||
| manager.setTimeout(callback, 100) | ||
| expect(customProvider.setTimeout).toHaveBeenCalledWith(callback, 100) | ||
|
|
||
| manager.clearTimeout(999) | ||
| expect(customProvider.clearTimeout).toHaveBeenCalledWith(999) | ||
|
|
||
| manager.setInterval(callback, 200) | ||
| expect(customProvider.setInterval).toHaveBeenCalledWith(callback, 200) | ||
|
|
||
| manager.clearInterval(888) | ||
| expect(customProvider.clearInterval).toHaveBeenCalledWith(888) | ||
| }) | ||
|
|
||
| it('warns when switching providers after making call', () => { | ||
| // 1. switching before making any calls does not warn | ||
| const customProvider = createMockProvider() | ||
| manager.setTimeoutProvider(customProvider) | ||
| expect(console.warn).not.toHaveBeenCalled() | ||
|
|
||
| // Make a call. The next switch should warn | ||
| manager.setTimeout(vi.fn(), 100) | ||
|
|
||
| // 2. switching after making a call should warn | ||
| const customProvider2 = createMockProvider('custom2') | ||
| manager.setTimeoutProvider(customProvider2) | ||
| expect(console.warn).toHaveBeenCalledWith( | ||
| '[timeoutManager]: Switching to custom2 provider after calls to custom provider might result in unexpected behavior.', | ||
| ) | ||
|
|
||
| // 3. Switching again with no intermediate calls should not warn | ||
| vi.mocked(console.warn).mockClear() | ||
| const customProvider3 = createMockProvider('custom3') | ||
| manager.setTimeoutProvider(customProvider3) | ||
| expect(console.warn).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
||
| it('throws if provider returns non-convertible value from setTimeout/setInterval', () => { | ||
| const invalidValue = { invalid: true } as any | ||
| const customProvider = createMockProvider('badProvider') | ||
| customProvider.setTimeout = vi.fn(() => invalidValue) | ||
| customProvider.setInterval = vi.fn(() => invalidValue) | ||
| manager.setTimeoutProvider(customProvider) | ||
|
|
||
| const callback = vi.fn() | ||
|
|
||
| expect(() => manager.setTimeout(callback, 100)).toThrow( | ||
| 'TimeoutManager: could not convert badProvider provider timeout ID to valid number', | ||
| ) | ||
|
|
||
| expect(() => manager.setInterval(callback, 100)).toThrow( | ||
| 'TimeoutManager: could not convert badProvider provider timeout ID to valid number', | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('globalThis timeoutManager instance', () => { | ||
| it('should be an instance of TimeoutManager', () => { | ||
| expect(timeoutManager).toBeInstanceOf(TimeoutManager) | ||
| }) | ||
| }) | ||
|
|
||
| describe('exported functions', () => { | ||
| let provider: ReturnType<typeof createMockProvider> | ||
| let callNumber = 0 | ||
| beforeEach(() => { | ||
| callNumber = 0 | ||
| provider = createMockProvider() | ||
| timeoutManager.setTimeoutProvider(provider) | ||
| }) | ||
| afterEach(() => { | ||
| timeoutManager.setTimeoutProvider(defaultTimeoutProvider) | ||
| }) | ||
|
|
||
| const callbackArgs = () => [vi.fn(), 100 * ++callNumber] as const | ||
|
|
||
| describe('managedSetTimeout', () => { | ||
| it('should call timeoutManager.setTimeout', () => { | ||
| const spy = vi.spyOn(timeoutManager, 'setTimeout') | ||
| const args = callbackArgs() | ||
|
|
||
| const result = managedSetTimeout(...args) | ||
|
|
||
| expect(spy).toHaveBeenCalledWith(...args) | ||
| expect(result).toBe(123) | ||
| }) | ||
| }) | ||
|
|
||
| describe('managedClearTimeout', () => { | ||
| it('should call timeoutManager.clearTimeout', () => { | ||
| const spy = vi.spyOn(timeoutManager, 'clearTimeout') | ||
| const timeoutId = 123 | ||
|
|
||
| managedClearTimeout(timeoutId) | ||
|
|
||
| expect(spy).toHaveBeenCalledWith(timeoutId) | ||
|
|
||
| spy.mockRestore() | ||
| }) | ||
| }) | ||
|
|
||
| describe('managedSetInterval', () => { | ||
| it('should call timeoutManager.setInterval', () => { | ||
| const spy = vi.spyOn(timeoutManager, 'setInterval') | ||
| const args = callbackArgs() | ||
|
|
||
| const result = managedSetInterval(...args) | ||
|
|
||
| expect(spy).toHaveBeenCalledWith(...args) | ||
| expect(result).toBe(456) | ||
| }) | ||
| }) | ||
|
|
||
| describe('managedClearInterval', () => { | ||
| it('should call timeoutManager.clearInterval', () => { | ||
| const spy = vi.spyOn(timeoutManager, 'clearInterval') | ||
| const intervalId = 456 | ||
|
|
||
| managedClearInterval(intervalId) | ||
|
|
||
| expect(spy).toHaveBeenCalledWith(intervalId) | ||
| }) | ||
| }) | ||
|
|
||
| describe('systemSetTimeoutZero', () => { | ||
| it('should use globalThis setTimeout with 0 delay', () => { | ||
| const spy = vi.spyOn(globalThis, 'setTimeout') | ||
|
|
||
| const callback = vi.fn() | ||
| systemSetTimeoutZero(callback) | ||
|
|
||
| expect(spy).toHaveBeenCalledWith(callback, 0) | ||
| clearTimeout(spy.mock.results[0]?.value) | ||
| }) | ||
| }) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: we want to change this to
scheduleMicroTaskin the next major version, so maybe we should keep this outside of the managed timeouts to avoid larger changes when someone opts into their own managed timeoutThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I have it calling
systemSetTimeoutZero- no change in semantics, and the code clearly shows that it's not an oversightThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you’re saying places where we now use
systemSetTimeoutZerowe should in the future usescheduleMicroTask?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I’m just now realizing that
systemSetTimeoutZerodoesn’t use thetimeoutManager😅 . I get it now 👍Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't make that assessment - it's up to you to decide that should remain
globalThis.setTimeout(cb, 0)and what should change to your newscheduleMicroTaskfunction.But yeah, my intention is that changes from
setTimeout(cb, 0)tosystemSetTimeoutZeroare syntactic only (to ease code review going forward), and remain semantically unchanged compared to before this PR.