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

fix: support file: and chrome-extension: protocols in client #2954

Merged
merged 1 commit into from Jan 3, 2021

Conversation

vespakoen
Copy link
Contributor

@vespakoen vespakoen commented Jan 2, 2021

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes, I added tests to check if the protocol was changed, it is poking at variables inside of WebSocket and SockJS though, this is probably not the most stable way to do this, but I am not sure how to mock those or if that is even necessary.

Motivation / Use-Case

When using node-webkit, or electron, the protocol can be file: or chrome-extension:, causing the websocket or sockjs to try to connect to an incorrect url, for example: file://localhost:8080/ws or chrome-extension://localhost:8080/ws.

This PR changes it to the correct behaviour.

Breaking Changes

This PR should not be a breaking change.

Additional Info

I just noticed I also replace https with ws, this should probably be an extra case where it is replaced with wss.
EDIT: I removed the https case

@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #2954 (e1ab7a4) into master (ca9e45d) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2954      +/-   ##
==========================================
- Coverage   92.54%   92.39%   -0.15%     
==========================================
  Files          37       37              
  Lines        1261     1263       +2     
  Branches      334      328       -6     
==========================================
  Hits         1167     1167              
- Misses         89       91       +2     
  Partials        5        5              
Impacted Files Coverage Δ
client-src/clients/SockJSClient.js 63.63% <100.00%> (-2.28%) ⬇️
client-src/clients/WebsocketClient.js 57.77% <100.00%> (-0.37%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca9e45d...e1ab7a4. Read the comment docs.

@vespakoen vespakoen force-pushed the fix-nwjs-compat branch 2 times, most recently from f20a98d to 25b92e3 Compare January 3, 2021 12:06
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.

None yet

3 participants