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.
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
Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552
base: master
Are you sure you want to change the base?
Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552
Changes from all commits
0cac2d7
ee1d17e
000c5cb
b089ffc
da33d79
264d775
2f186ee
753c410
6fdf33a
4071797
edd5bb5
cda254e
96bedc3
040bf62
bd82c58
6bed4f6
8d9c2b2
1a04106
f3d0b08
0277135
6f39061
54a6964
793382c
b85ea1e
d17ad79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does
UAParser
not have these kinds of safety checks? If you wrote it this way because tonavigator.userAgent
"might be undefined", let UAParser find the user-agent string? Thenew
will also avoid the need for the eslint annotationI think your implementation/invocation is useful when you're running it in a node context or analyzing data.
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.
Is there a way to differentiate between/among these types of errors?
Seems to me
isNetworkError
will be true in all those cases and retry the fetch, although we don't want to retry on CORS, non-existent host, etc.LMK if I'm misreading/misunderstanding the behavior
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.
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.
Probably good to add some tests to ensure we don't catch and retry CORS errors, etc
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.
You're correct,
isNetworkError
will be true in all those 3 cases.There is no way to distinguish programmatically in JS.
(The difference can be seen only in browser console)
That's why I added
canRetry
option to enable retry logic only for polling requests.Polling requests are repeatable by design so I think it's fine to retry it due to real network loss.
Since all API requests should be performed to the same host and poll request is never a first request in a list of ones performed by SIW, possible CORS error should be caught before polling is started.
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.
Thanks. Is there any logic to say we should stop trying polling after n failed attempts?