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.
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.
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 |
evanweible-wf
left a comment
There was a problem hiding this comment.
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: