-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Extract ClientStream into its own package. #9371
Conversation
5e872a6
to
545e420
Compare
// the client, as desired. | ||
const modulePath = Meteor.isClient | ||
? '../client/stream_client_sockjs' | ||
: '../server/stream_client_nodejs'; |
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.
@stubailo I was able to get rid of this trickery by using different client/server entry points in a separate package.
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.
Looks great @benjamn - love this decoupling!
@benjamn if you implement this, leave the door opened to allow using Meteor without a websocket connection so we can fetch the modules via an http request. |
@@ -114,8 +113,8 @@ export default class ClientStream extends StreamClientCommon { | |||
} | |||
|
|||
_heartbeat_timeout() { | |||
Meteor._debug('Connection timeout. No sockjs heartbeat received.'); | |||
this._lostConnection(new DDP.ConnectionError('Heartbeat timed out')); |
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.
It's a shame this custom error type used a special errorType
property.
It seems like we mainly check for this type of error internally, notably tools/meteor-services/auth-client.js#L33
and a couple places in tools/packaging/catalog/catalog.js
, but this presumably bubbles into those under some circumstances? Perhaps those should be updated?
Other than that, I did some GitHub code searching to try to hone in on any potential problems, but didn't come across anything concerning or that might be a breaking change, though I suppose developers could be looking for this type of error in their own private code?
@@ -201,7 +200,7 @@ export default class ClientStream extends StreamClientCommon { | |||
|
|||
if (this.connectionTimer) clearTimeout(this.connectionTimer); | |||
this.connectionTimer = setTimeout(() => { | |||
this._lostConnection(new DDP.ConnectionError('DDP connection timed out')); | |||
this._lostConnection(new Error("DDP connection timed out")); |
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.
Should we start re-branding this error without DDP
in it?
@theodorDiaconu That door will always stay open (just need to swap out this |
@benjamn I also noticed that http request will be slower than ddp, but I've recently published a POC package that allows conditionally starting websocket only when you need it. https://github.com/cult-of-coders/fusion it's hackish but it does the job. I would very much like this to be in the core, or allow a meteor package to hack it somehow... The reason for this is so Meteor can also address the market of apps that don't require a stateful relation with the client. |
I'm intentionally leaving forcedReconnectError as a non-configurable singleton Error object.
545e420
to
fe05aa7
Compare
Right now, client-side access to Meteor's persistent WebSocket connection is controlled by the
ddp-client
package, which unfortunately includes some large modules likelivedata_connection.js
that help implement DDP and Livedata, but do not seem necessary for simple socket-based communication with the server.This PR is the beginning of an effort to make the WebSocket connection usable without also depending on the
ddp-client
package.For example, eventually the
dynamic-import
package might be able to depend directly onsocket-stream-client
to fetch dynamic modules via WebSocket, without depending on all ofddp-client
as well. This would allow dynamicimport()
to be used earlier in the Meteor startup process, in more fundamental ways, regardless of whetherddp-client
is used by the application.