-
Notifications
You must be signed in to change notification settings - Fork 120
fix: Clamp duration to setTimeout max value #381
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1018,4 +1018,51 @@ describe('Notification.Basic', () => { | |||||||||||
| 'xxx', | ||||||||||||
| ); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| describe('MAX_DURATION', () => { | ||||||||||||
| it('should not warn when duration is within limit', () => { | ||||||||||||
| const { instance } = renderDemo(); | ||||||||||||
| const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||||||||||||
|
|
||||||||||||
| act(() => { | ||||||||||||
| instance.open({ | ||||||||||||
| content: <p className="test">1</p>, | ||||||||||||
| duration: 0.1, | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| expect(document.querySelector('.test')).toBeTruthy(); | ||||||||||||
| expect(errSpy).not.toHaveBeenCalled(); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| it('should warn and clamp when duration exceeds limit', () => { | ||||||||||||
| const { instance } = renderDemo(); | ||||||||||||
| const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); | ||||||||||||
|
|
||||||||||||
| const MAX_DURATION = 2147483647 / 1000; | ||||||||||||
| const EXCEEDED_DURATION = (2147483647 + 1) / 1000; | ||||||||||||
|
Comment on lines
+1042
to
+1043
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To improve maintainability and avoid repeating the magic number
Suggested change
|
||||||||||||
|
|
||||||||||||
| act(() => { | ||||||||||||
| instance.open({ | ||||||||||||
| content: <p className="test">1</p>, | ||||||||||||
| duration: EXCEEDED_DURATION, | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
|
|
||||||||||||
| expect(document.querySelector('.test')).toBeTruthy(); | ||||||||||||
| expect(errSpy).toHaveBeenCalledWith( | ||||||||||||
| `Warning: \`duration\` exceeds the maximum supported value (${MAX_DURATION}s) and has been clamped.`, | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| act(() => { | ||||||||||||
| vi.advanceTimersByTime(MAX_DURATION * 1000 - 1); | ||||||||||||
| }); | ||||||||||||
| expect(document.querySelector('.test')).toBeTruthy(); | ||||||||||||
|
|
||||||||||||
| act(() => { | ||||||||||||
| vi.advanceTimersByTime(1); | ||||||||||||
| }); | ||||||||||||
| expect(document.querySelector('.test')).toBeFalsy(); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
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.
To improve readability and avoid a magic number, it would be clearer to express
2147483647as the calculation it represents. This makes the origin of the number apparent directly in the code, aligning with the explanation in the comment and the PR description.