Skip to content
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

WP-5258 Automatically use sockjs_client_wrapper if possible. #296

Merged
merged 2 commits into from
Jan 10, 2018

Conversation

evanweible-wf
Copy link
Contributor

@evanweible-wf evanweible-wf commented Jan 4, 2018

Changes

With the release of sockjs_client_wrapper, we want to use that library if possible because it is fully featured and supports all of the SockJS protocols (particularly xhr-polling). But, we can't just switch because it requires the addition of the sockjs.js library to the HTML page.

The proposed solution here is to check for the existence of the sockjs.js script by checking for SockJS on the window. If it's there, we'll automatically leverage the new package instead of the old Dart port. Otherwise, we'll fallback to the original behavior (i.e. nothing should change).

Testing

Code Review

@Workiva/app-frameworks

@evanweible-wf evanweible-wf requested a review from a team as a code owner January 4, 2018 18:21
@aviary2-wf
Copy link

aviary2-wf commented Jan 4, 2018

Raven

Number of Findings: 0

noCredentials: noCredentials == true,
// If the sockjs.js file wasn't detected, then we fallback to the original
// SockJS Dart port.
return SockJSPortWebSocket.connect(uri,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on spitting out a hint about using the new lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note to the changelog, the source docs for WebSocket, and the SockJS guide. I'd rather not log/print anything right now since I don't see us making the major jump anytime soon and in the meantime it's valid for consumers to not want to change anything since it still works.

@evanweible-wf evanweible-wf force-pushed the auto_use_sockjs_client_wrapper branch 2 times, most recently from 5b4a2ee to a2e6d22 Compare January 5, 2018 16:30
@evanweible-wf evanweible-wf reopened this Jan 8, 2018
@evanweible-wf evanweible-wf reopened this Jan 8, 2018
@evanweible-wf evanweible-wf force-pushed the auto_use_sockjs_client_wrapper branch from a2e6d22 to 750e1b0 Compare January 9, 2018 16:02
@codecov-io
Copy link

codecov-io commented Jan 9, 2018

Codecov Report

Merging #296 into master will increase coverage by 1.6%.
The diff coverage is 94.66%.

@jayudey-wf
Copy link
Contributor

jayudey-wf commented Jan 10, 2018

QA +1

  • Testing instruction
  • Dev +1's
  • Dev/QA +10 with detail of what was tested
    • verified in the wdesk example that it was still able to function
    • verified in echo example that messages were still able to be sent
  • Security review (if required)
  • Unit tests created/updated
  • All unit tests pass
  • Rosie has run and reports properly the release the ticket will be included in

Merging into master.

@jayudey-wf jayudey-wf merged commit a928546 into master Jan 10, 2018
@maxwellpeterson-wf maxwellpeterson-wf deleted the auto_use_sockjs_client_wrapper branch January 10, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants