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

Update ParseWebSocketServer.js #5860

Merged
merged 3 commits into from
Jul 29, 2019
Merged

Update ParseWebSocketServer.js #5860

merged 3 commits into from
Jul 29, 2019

Conversation

grosscorporation
Copy link

@grosscorporation grosscorporation commented Jul 28, 2019

fix wss:// error by requiring 'ws' module, remove uws as it has been deprecated and removed from npm

See: #5347

fix wss:// error by requiring 'ws' module, remove uws as it has been deprecated and removed from npm
@dplewis
Copy link
Member

dplewis commented Jul 28, 2019

I've been thinking about this for a while. I sure you know why this package was removed from npm and the drama behind it. Deprecation doesn't mean we can't use it. uws is still significantly faster and efficient than ws.

I propose a few options:

Migrate to uWebSocket.js

Its not a direct 1-1 transition because a wrapper would need to be implemented. This package is a rewrite of uws, but I'm skeptical because of the history of the author.

Add wsModule option to server config

{
  wsModule: 'ws', // default or 'uws'
}

Allow users to implement there own WS Adapters

There are many Websocket packages like socket.io that can be used as a substitute for ws. This will allow for plug and play interface.

Debug your issue / check your configuration

const getWS = function() {
  try {
    return require('uws');
  } catch (e) {
    return require('ws'); <- should catch error if using node 10+ since uws binds to 8 and 9.
  }
};

You can build this maintained fork and compare your .node bindings

$ ls node_modules/uws
LICENSE                 uws_darwin_57.node*     uws_linux_59.node*      uws_win32_64.node*
README.md               uws_darwin_59.node*     uws_linux_64.node*
package.json            uws_darwin_64.node*     uws_win32_57.node*
uws.js                  uws_linux_57.node*      uws_win32_59.node*
~/src/Parse/parse-server (master ±)

Remove it entirely

The JS-SDK doesn't use uws either, I thought about adding it but would like the opinion of others.

@GoGross @TomWFox @davimacedo @acinader Thoughts?

@codecov
Copy link

codecov bot commented Jul 28, 2019

Codecov Report

Merging #5860 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5860      +/-   ##
==========================================
- Coverage   93.68%   93.65%   -0.03%     
==========================================
  Files         150      150              
  Lines       10616    10614       -2     
==========================================
- Hits         9946     9941       -5     
- Misses        670      673       +3
Impacted Files Coverage Δ
src/LiveQuery/ParseWebSocketServer.js 94.73% <100%> (-0.51%) ⬇️
src/RestWrite.js 93.39% <0%> (-0.5%) ⬇️

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 80bb2b6...20ea330. Read the comment docs.

@grosscorporation
Copy link
Author

@dplewis how about removing it entirely and as you mentioned above, allow WS Adapters which would be awesome.

@dplewis
Copy link
Member

dplewis commented Jul 28, 2019

I updated your PR. I also added swappable WebSocketController to the JS-SDK

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this. I'll work on the Adapter.

@dplewis dplewis merged commit 4f21c36 into parse-community:master Jul 29, 2019
@davimacedo
Copy link
Member

@dplewis I've just seen the thread and I think you guys took a good approach. Let me know if you need any help with the adapter.

@ramiro-ciocca
Copy link

Thanks guys! This solved a big issue with live queries in my app, when device goes offline then back online. It wouldnt resusbscribe the live query because of the "dead" connection when going offline. With this update, dead connections dont interfere with the new one when network is back.

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* Update ParseWebSocketServer.js

fix wss:// error by requiring 'ws' module, remove uws as it has been deprecated and removed from npm

* Update ParseWebSocketServer.js

* remove uws
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.

4 participants