Reconnecting Websocket Example and Updated Readme #138

mbalex99 · 2017-01-26T22:12:25Z

I updated the textarea example and updated the readme about how to handle reconnections.

coveralls · 2017-01-26T22:14:10Z

Coverage remained the same at 96.106% when pulling e1f7ab0 on mbalex99:reconnectingwebsocket into 05c30eb on share:master.

curran

Nice change.

curran · 2017-01-27T07:51:07Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions


Nice! It's high time this documentation be updated.

Actually, as per #121, this is correct.

When the WebSocket reconnects, the Connection middleware reconnects both documents and query subscriptions.

We might need more clear documentation, but this line is correct.

Perhaps a more clear wording would be - Reconnection of document and query subscriptions on socket reconnection (Check 'Reconnection' section below). Thoughts?

@pedrosanta I think that would be more clear. I'd welcome that addition here.

@nateps This PR seems to address a common source of frustration/confusion on ShareDB reconnection. Any chance it could be reviewed by the team?

I agree that the frequent confusion indicates we should change the wording.

However, I would like to highlight the fact that subscriptions state is re-established as soon as the connection resumes. This is a key feature of the ShareDB client.

I prefer the suggestion of more clear wording.

curran · 2017-04-04T09:11:12Z

Anyone reviewing these PRs? This change looks good to me.

curran · 2017-05-12T00:11:12Z

examples/textarea/client.js

+
+element.style.backgroundColor = "gray";
+socket.onopen = function(){
+  status.innerHTML = "Connected"


This is quite useful stuff!

curran · 2017-05-12T00:12:04Z

@nateps @rkstedman This PR looks like a nice improvement. Would merge it if I could. Any chance one of you could review this one? Thank you.

pedrosanta

~~Overall the code seems pretty nice @mbalex99, nice one. 👍 But I don't know if we, like, shouldn't really make the effort and add this (the reconnect handling) built-in on Connection/ShareDB.~~

Comment retracted, check new review.

pedrosanta · 2017-05-19T01:50:32Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions
 - Offline change syncing upon reconnection


~~@mbalex99 I mean, if reconnection doesn't come out of the box, I think this line should also be removed, as it's misleading.~~

Actually I retract my comment, this actually happens too.

Ops are added to pending 'queue'/array and flushed/synced on socket reconnection.

This line is also correct.

Also, a better wording could be - Offline change syncing upon socket reconnection.

pedrosanta

Also we probably need to update connection.js comment here, as it seems misleading:

https://github.com/share/sharedb/blob/master/lib/client/connection.js#L90

Perhaps update to something like:

  // - 'disconnected' Connection is closed, and reconnect can be attempted
  // - 'closed'       The connection was closed by the client, a reconnect should not be attempted
  // - 'stopped'      The connection was closed by the server, a reconnect should not be attempted

Also, perhaps we should update the documentation regarding node-browserchanel here, wdyt?

https://github.com/share/sharedb/blob/master/lib/client/connection.js#L138

For more on this check #121 too.

pedrosanta · 2017-05-23T13:13:59Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions


Actually, as per #121, this is correct.

When the WebSocket reconnects, the Connection middleware reconnects both documents and query subscriptions.

We might need more clear documentation, but this line is correct.

pedrosanta · 2017-05-23T13:26:00Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions
 - Offline change syncing upon reconnection


Actually I retract my comment, this actually happens too.

Ops are added to pending 'queue'/array and flushed/synced on socket reconnection.

This line is also correct.

pedrosanta · 2017-05-23T13:34:37Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions


Perhaps a more clear wording would be - Reconnection of document and query subscriptions on socket reconnection (Check 'Reconnection' section below). Thoughts?

pedrosanta · 2017-05-23T13:36:25Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions
 - Offline change syncing upon reconnection


Also, a better wording could be - Offline change syncing upon socket reconnection.

curran · 2017-06-07T05:51:20Z

@nateps @rkstedman Any chance of reviewing this PR? I find it to be a great improvement.

pedrosanta · 2017-06-07T08:21:31Z

@curran @nateps @rkstedman I think this PR still needs some updates/improvements though, as I've commented.

It was great if @mbalex99 could address those. 🙂

curran · 2017-06-07T09:45:58Z

@pedrosanta Indeed, thanks for your suggestions. The discussion is quite long, and it will take some work to go through and aggregate the updates/improvements you suggested.

Would you mind summarizing the specific changes you'd like to see?

(or, better yet, maybe you could branch off this and create a new PR with your proposed changes 😉 )

curran · 2017-06-10T06:08:53Z

@pedrosanta To summarize your proposed changes from the discussion:

Instead of deleting Reconnection of document and query subscriptions, replace it with Reconnection of document and query subscriptions on socket reconnection (Check 'Reconnection' section below)
Change Offline change syncing upon reconnection to Offline change syncing upon socket reconnection.
in https://github.com/share/sharedb/blob/master/lib/client/connection.js#L90, change

  // - 'disconnected' Connection is closed, but it will reconnect automatically
  // - 'closed'       The connection was closed by the client, and will not reconnect
  // - 'stopped'      The connection was closed by the server, and will not reconnect

to

  // - 'disconnected' Connection is closed, and reconnect can be attempted
  // - 'closed'       The connection was closed by the client, a reconnect should not be attempted
  // - 'stopped'      The connection was closed by the server, a reconnect should not be attempted

Are these the complete changes to this PR you'd like to see? It almost feels like these changes could be made in a separate documentation-focused PR, after this PR gets merged. Thoughts?

curran · 2017-11-24T09:52:09Z

@nateps @rkstedman @josephg [PR tending note] I think this PR can be merged. It would close the long-standing issue #121 , which would be a big win. Changes are in documentation and examples only, so no risk of ShareDB breakage.

nateps

Appreciate the contribution!

There are two ways to implement reconnection with ShareDB Connections, either using a WebSocket-like object that implements reconnection or calling Connection::bindToSocket() with a new WebSocket object. I'll merge this and document that both options are supported.

nateps · 2018-07-14T18:47:39Z

README.md

@@ -27,10 +27,26 @@ tracker](https://github.com/share/sharedb/issues).
 - Projections to select desired fields from documents and operations
 - Middleware for implementing access control and custom extensions
 - Ideal for use in browsers or on the server
- Reconnection of document and query subscriptions


I agree that the frequent confusion indicates we should change the wording.

However, I would like to highlight the fact that subscriptions state is re-established as soon as the connection resumes. This is a key feature of the ShareDB client.

I prefer the suggestion of more clear wording.

nateps · 2018-07-14T19:04:50Z

README.md

+
+The easiest way is to give it a WebSocket object that does reconnect. There are plenty of example on the web. The most important thing is that the custom reconnecting websocket, must have the same API as the native rfc6455 version.
+
+In the "textarea" example we show this off using a Reconnecting Websocket implementation from [https://github.com/pladaria/reconnecting-websocket](reconnecting-websocket).


This is a great README addition, and it clarifies one way that ShareDB Connections support reconnection.

I'd also like to highlight that it is possible and intended that reconnection is implemented by calling Connection::bindToSocket() with a new WebSocket instance, as @DetweilerRyan pointed out. We've chosen not to implement reconnection logic in ShareDB, itself. There are other great libraries for that purpose, and application developers may need to leverage different libraries or custom logic depending on their requirements.

Reading the code of Reconnecting WebSocket, a significant amount of its complexity is necessitated by the constraint of exposing a single instance externally and internally creating new instances of WebSocket objects. If the reconnection logic of Reconnecting WebSocket implementation works for you, this is totally fine!

However, if you want to implement custom reconnection logic for your production environment (say you want to do backoff in a different way or fallback to other protocols or libraries), calling the ShareDB bindToSocket() with a new WebSocket or WebSocket-like instance might be simpler.

curran · 2018-07-16T12:27:56Z

Woohoo thanks @nateps !

mbalex99 added 2 commits January 26, 2017 14:11

adding reconnecting websocket example in text area and updating readme

78306cc

Updating Readme

e1f7ab0

curran reviewed Jan 27, 2017

View reviewed changes

curran reviewed May 12, 2017

View reviewed changes

curran mentioned this pull request May 12, 2017

Connection does not reconnect automatically #121

Closed

pedrosanta approved these changes May 19, 2017

View reviewed changes

pedrosanta suggested changes May 23, 2017

View reviewed changes

curran mentioned this pull request Nov 24, 2017

More maintainers required? #163

Closed

rkstedman assigned nateps Dec 1, 2017

nateps approved these changes Jul 14, 2018

View reviewed changes

nateps merged commit 762e05d into share:master Jul 14, 2018

mbalex99 deleted the reconnectingwebsocket branch July 18, 2018 01:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconnecting Websocket Example and Updated Readme #138

Reconnecting Websocket Example and Updated Readme #138

mbalex99 commented Jan 26, 2017

coveralls commented Jan 26, 2017 •

edited

Loading

curran left a comment

curran Jan 27, 2017

pedrosanta May 23, 2017

pedrosanta May 23, 2017

curran May 25, 2017

nateps Jul 14, 2018

curran commented Apr 4, 2017

curran May 12, 2017

curran commented May 12, 2017

pedrosanta left a comment •

edited

Loading

pedrosanta May 19, 2017 •

edited

Loading

pedrosanta May 23, 2017

pedrosanta May 23, 2017

pedrosanta left a comment •

edited

Loading

pedrosanta May 23, 2017

pedrosanta May 23, 2017

pedrosanta May 23, 2017

pedrosanta May 23, 2017

curran commented Jun 7, 2017

pedrosanta commented Jun 7, 2017 •

edited

Loading

curran commented Jun 7, 2017 •

edited

Loading

curran commented Jun 10, 2017

curran commented Nov 24, 2017 •

edited

Loading

nateps left a comment

nateps Jul 14, 2018

nateps Jul 14, 2018

curran commented Jul 16, 2018


		The easiest way is to give it a WebSocket object that does reconnect. There are plenty of example on the web. The most important thing is that the custom reconnecting websocket, must have the same API as the native rfc6455 version.

		In the "textarea" example we show this off using a Reconnecting Websocket implementation from [https://github.com/pladaria/reconnecting-websocket](reconnecting-websocket).

Reconnecting Websocket Example and Updated Readme #138

Reconnecting Websocket Example and Updated Readme #138

Conversation

mbalex99 commented Jan 26, 2017

coveralls commented Jan 26, 2017 • edited Loading

curran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curran commented Apr 4, 2017

Choose a reason for hiding this comment

curran commented May 12, 2017

pedrosanta left a comment • edited Loading

Choose a reason for hiding this comment

pedrosanta May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrosanta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curran commented Jun 7, 2017

pedrosanta commented Jun 7, 2017 • edited Loading

curran commented Jun 7, 2017 • edited Loading

curran commented Jun 10, 2017

curran commented Nov 24, 2017 • edited Loading

nateps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curran commented Jul 16, 2018

coveralls commented Jan 26, 2017 •

edited

Loading

pedrosanta left a comment •

edited

Loading

pedrosanta May 19, 2017 •

edited

Loading

pedrosanta left a comment •

edited

Loading

pedrosanta commented Jun 7, 2017 •

edited

Loading

curran commented Jun 7, 2017 •

edited

Loading

curran commented Nov 24, 2017 •

edited

Loading