-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add timeout option #45
Conversation
This can't actually merge, since I manually made changes to sockjs.js, but the code changes are there because I needed to be able to test with https://github.com/Workiva/doc_plat_client/pull/30069 |
Security InsightsThe items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact. (1) Security relevant changes were detectedlib/sockjs.js modifiedAction Items
Questions or Comments? Reach out on Slack: #support-infosec. |
if (timeout === undefined) { | ||
timeout = InfoReceiver.timeout |
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.
If sockJS's internally calculated
/// timeout is higher, that will be used instead.
😕 if true, shouldn't this conditional be < ? But maybe also set a lower limit?
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, it's min timeout!
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.
Oh, whoops, I must have had that slip through the cracks somewhere. I'll see what the sockjs maintainer says. (I don't know if they will want this or that)
Looks like |
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.
LGTM pending that one question about the timeout and the upstream PR.
Couldn't get sockjs to look at my PR. sockjs/sockjs-client#594 |
Motivation
I hope to make the timeout on InfoReceiver handshake configurable. sockjs/sockjs-client#594
When that happens, the wrapper will need its api updated to actually pass in the timeout option to SockJS.
Changes
Add the timeout to the SockJsOptions
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: