Refactor network wrappers #243

ballinette · 2017-08-11T11:41:37Z

Improvements

Refactor the network wrappers to improve separation of concerns:
- simplification of network states (offline, connecting, connected for realtime protocole ; offline, ready for future HTTP protocol)
- each protocol is responsible of its states, and manage itself the offline mode
- Kuzzle object still exposes queuing methods, but now delegates their implementation to network protocols.
Refactor the Room management : the Room class now inherits from EventEmitter:
- a room handles the subscription to Kuzzle API (one single subscription for each room, with its own filters and options)
- listener callbacks can subscribe to a room to be notified by Kuzzle notifications.

Some sample codes to manage rooms:

var 
  collection = kuzzle.collection('foo', 'bar'),
  filters = {equals: {foo: 'bar'}},
  room;

/*
* simple subscribe to new document with default scope/state options
*/
room = collection.subscribe(filters, function(data) {
  console.log('New document within the scope: ', data.document);
}).onDone(function(err, res) {
  if (err) {
    console.error('Error while subscribing to the room: ', err);
  } else {
    console.log('Subscription ready');
  }
});

/*
* Create a Room with custom options and add some listeners to it
*/

// create the room:
room = collection.room(filters, {state: all, scope: all, users: all, subscribeToSelf: false});

// listen to notifications about documents:
room.on('document', function(data) {
  if (data.scope === 'in') {
    console.log('New document within the scope: ', data.document);
  } else if (data.scope === 'out') {
    console.log('Document moved from the scope: ', data.document);
  } 
});

// listen to notifications about other users subscribing to the same room:
room.on('user', function(data) {
  if (data.user === 'in') {
    console.log('A user has joigned the room', data.volatile);
  } else if (data.user == 'out') {
    console.log('A user has leaved the room', data.volatile);
  }
  console.log('Number of listening users: ', data.result.count);
});

// subscribe to the room:
room.subscribe(function(err, res) {
  if (err) {
    console.error('Error while subscribing to the room: ', err);
  } else {
    console.log('Subscription ready');
  }
});

Boyscout improvements

use ES6 and Babel to browser transpilation (only for new and refactored classes for now)
promisify somme missing method of Kuzzle and Room prototypes
replace jwtToken by jwt
improve unit tests

Breaking changes

Renamed properties and method in `Kuzzle` prototype

jwtToken => jwt
getJwtToken() => getJwt()
setJwtToken(token)' => setJwt(token)`
unsetJwtToken() => unsetJwt()
replayQueue() => playQueue()

Removal of `SubscribeResult` prototype:

onDone method move to Room prototype.
Collection.subscribte() method returns now the created Room object

Changed signature for `Collection.room` method and `Room` constructor

collection.room([options]) => collection.room(filters, [options])
new Room(collection, [options]) => new Room(collection, filters, [options])

Changed signature for `Room.renew` method

renew([filters], notificationCallback, subscriptionCallback) => renew(subscriptionCallback)

filters is now an immutable property (we need to create a new Room if we want to change the filters)
notificationCallback has become useless since the Room is now an event emitter:

// Old code:
room.renew(notificationCallback, subscriptionCallback);

// ===> New Code:
room.renew(subscriptionCallback).addListener('document', notificationCalllback);

New option `protocol` to Kuzzle constructor

Must be filled to use another protocol than native websocket
currently, 2 supported values:

websocket (default)
socketio
⚠️ The network wrappers does not automatically fallback to socketIO anymore if Websocket is not supported.

codecov-io · 2017-08-11T11:51:53Z

Codecov Report

Merging #243 into 6.x will increase coverage by 0.31%.
The diff coverage is 99.55%.

@@            Coverage Diff             @@
##              6.x     #243      +/-   ##
==========================================
+ Coverage   98.27%   98.58%   +0.31%     
==========================================
  Files          17       17              
  Lines        2149     2127      -22     
  Branches      611      615       +4     
==========================================
- Hits         2112     2097      -15     
+ Misses         37       30       -7

Impacted Files	Coverage Δ
src/eventEmitter/index.js	`92.47% <100%> (ø)`	⬆️
src/networkWrapper/index.js	`100% <100%> (ø)`	⬆️
src/networkWrapper/protocols/websocket.js	`100% <100%> (ø)`
src/Room.js	`100% <100%> (ø)`	⬆️
src/Collection.js	`100% <100%> (+1.95%)`	⬆️
src/networkWrapper/protocols/socketio.js	`100% <100%> (ø)`
src/Kuzzle.js	`98.52% <98.88%> (-0.25%)`	⬇️
src/networkWrapper/protocols/abstract/realtime.js	`99.29% <99.29%> (ø)`
... and 2 more

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 db3108f...9b81c95. Read the comment docs.

ballinette · 2017-08-11T12:41:13Z

src/Collection.js


  this.kuzzle.callbackRequired('Collection.scrollSpecifications', cb);

-  if (options && options.scroll) {


Note: not needed here since scroll option is already managed within kuzzle.querymethod

ballinette · 2017-08-11T12:41:24Z

src/Collection.js


  request.scrollId = scrollId;

-  if (options && options.scroll) {


Note: not needed here since scroll option is already managed within kuzzle.querymethod

ballinette · 2017-08-18T15:15:04Z

src/eventEmitter/index.js


    if (!listeners || !listeners.length) {
-      return;
+      return false;


Note: fix KuzzleEventEmitter methods signatures, to be consistant with NodeJS event specitications (emit should return a boolean value - see https://nodejs.org/api/events.html#events_emitter_emit_eventname_args )

scottinet

A few nitpickings here and there, but apart from that this looks good to me. Though I have a hard time to figure out how all this will play together. I trust you on that :-)

scottinet · 2017-08-22T13:30:52Z

src/Kuzzle.js

+        object.jwt = this.jwt;
+      }
+
+      if (room.volatile) {


This entire if block and the sdkVersion assignment below can be replaced with this one-liner:

Object.assign(object.volatile, room.volatile, {sdkVersion: this.sdkVersion});

(Object.assign will ignore undefined values so it's safe to use without testing for the existence of this object inside room)

scottinet · 2017-08-22T13:33:55Z

src/Kuzzle.js

+        object.jwt = this.jwt;
+      }
+
+      if (room.volatile) {


scottinet · 2017-08-22T13:36:52Z

src/Kuzzle.js

-  self.network.onConnectError(function (error) {
-    var connectionError = new Error('Unable to connect to kuzzle proxy server at "' + self.host + ':' + self.port + '"');
+  this.network.addListener('networkError', function (error) {
+    var connectionError = new Error('Unable to connect to kuzzle proxy server at "' + self.network.host + ':' + self.network.port + '"');


Nitpicking 1: use const instead of var
Nitpicking 2: use template litterals :-)

scottinet · 2017-08-22T13:38:49Z

src/Room.js

+      return this.kuzzle.bluebird.promisifyAll(this, {
+        suffix: 'Promise',
+        filter: function (name, func, target, passes) {
+          var whitelist = ['count', 'renew', 'subscribe', 'unsubscribe', 'onDone'];


Nitpicking: use const instead of var

scottinet · 2017-08-22T13:39:03Z

src/Room.js

+   * @param {responseCallback} cb - Handles the query response
+   */
+  count(cb) {
+    var data;


Nitpicking: use const instead of var ( data can be declared directly line 119 instead of here)

scottinet · 2017-08-22T13:40:43Z

src/Room.js

+   * @return {*} this
+   */
+  renew(cb) {
+    var


Nitpicking: use const instead of var

scottinet · 2017-08-22T14:27:57Z

src/networkWrapper/wrappers/abstract/realtime.js

+ * Clean history from requests made more than 10s ago
+ */
+function cleanHistory (requestHistory) {
+  var


Nitpicking: consider using const instead of var.

scottinet · 2017-08-22T14:28:53Z

src/networkWrapper/wrappers/abstract/realtime.js

+  var
+    now = Date.now();
+
+  Object.keys(requestHistory).forEach(function (key) {


Nitpicking: replace with for(const ... of ...)

scottinet · 2017-08-22T14:31:37Z

src/networkWrapper/wrappers/websocket.js

-      var reason = message;
+    this.client.onclose = (closeEvent, message) => {
+      let
+        error,


This error variable is shadowed by the one declared in the function starting line 65.

Suggestion: declare this variable as a const directly line 58, as it seems to be the only block where it is used.

scottinet · 2017-08-22T14:32:40Z

src/networkWrapper/wrappers/websocket.js

-    this.client.onerror = function (error) {
+    this.client.onerror = error => {
      if (!(error instanceof Error)) {
        error = new Error(error);


Nitpicking: function parameters should not be overwritten.

…rom Kuzzle root component to Network wrappers

* remove isValid() (obsolete method) * move queue management from Kuzzle to network wrappers * fix unit tests

+ use Babel for browsers

…JS `event` specitications: emit should return a boolean value. (see https://nodejs.org/api/events.html#events_emitter_emit_eventname_args )

…ead of the whole room) * add unit-tests for network.subscribe and network.unsubscribe methods

+ add 'use strict' in pure ES6 classes

to enable calls like `kuzzle.queryPromise({controller: 'server', action: 'healthCheck'}).then(...);`

xbill82 · 2017-09-12T14:47:57Z

src/networkWrapper/index.js

+      }
+      return new (require('./protocols/websocket'))(host, options);
+    case 'socketio':
+      if (!window.io) {


We assume that no one will ever try to use Socket.IO outside a browser?
(after all, sounds fair to me, just asking to be sure it's on purpose)

If we are outside a browser, we have native websocket support, so I don't see any reason to use socketIO...

dbengsch · 2017-09-12T14:46:14Z

test/kuzzle/connect.test.js

    });

-    it('should dequeue requests automatically on a connection success', function (done) {
+    it('should keep a valid JWT Token at reconnection', function () {


a valid JWT ?

dbengsch · 2017-09-12T14:46:36Z

test/kuzzle/connect.test.js


-  describe('=> on reconnection', function () {
-    it('should exit offline mode when reconnecting', function (done) {
+    it('should empty the JWT Token at reconnection if it has expired', function () {


dbengsch · 2017-09-12T14:48:24Z

test/kuzzle/methods.test.js

-      should((function () {kuzzle.setDefaultIndex(null);})).throw();
-      should((function () {kuzzle.setDefaultIndex(undefined);})).throw();
+  describe('#getJwt', function () {
+    it('should return the current jwt token', function () {


dbengsch · 2017-09-12T14:49:49Z

test/kuzzle/methods.test.js

-      });
+      should(kuzzle.jwt).be.undefined();
+      should(loginAttemptStub).be.calledOnce();
+      should(loginAttemptStub).be.calledWith({ error: 'Cannot find a valid JWT token in the following object: {"foo":"bar"}', success: false });


xbill82

Ok for me!

# Conflicts: # src/Kuzzle.js # test/network/networkWrapper.test.js

ballinette added changelog:breaking-changes changelog:enhancements wip labels Aug 11, 2017

ballinette commented Aug 11, 2017

View reviewed changes

ballinette commented Aug 18, 2017

View reviewed changes

ballinette added need-documentation and removed wip labels Aug 18, 2017

ballinette mentioned this pull request Aug 18, 2017

Document SDK Network and Room refactoring kuzzleio/documentation-old#350

Open

scottinet suggested changes Aug 22, 2017

View reviewed changes

ballinette force-pushed the refactor-network-wrappers branch from 6bdd4c5 to 0b5979a Compare August 22, 2017 14:56

ballinette added 18 commits August 29, 2017 19:03

Network wrappers refactoring: move persistent connection management f…

b57f6e8

…rom Kuzzle root component to Network wrappers

Network wrappers refactoring:

2c49779

* remove isValid() (obsolete method) * move queue management from Kuzzle to network wrappers * fix unit tests

some boyscout improvements on unit tests.

6e8fac0

rename jwtToken => jwt

58d6534

Network wrappers refactoring: add netowk events propagation

0daff75

Use ES6 syntax for network wrappers

986235e

+ use Babel for browsers

Refactor Room management - step 1

5750118

fix linter

bd7251e

improve code coverage

e847640

fix KuzzleEventEmitter methods signatures, to be consistant with Node…

8653d2b

…JS `event` specitications: emit should return a boolean value. (see https://nodejs.org/api/events.html#events_emitter_emit_eventname_args )

Refactor Kuzzle constructor and methods unit tests

0536828

readd Collection.room method + fix unit tests for Collection class

6115ae5

move requestHistory from Kuzzle to network wrappers

33cb738

remove all room listeners when unsuscribing

028c7c5

nitpicking

51a1c41

* change signature for network.unsubscribe (use the channel name inst…

47eb936

…ead of the whole room) * add unit-tests for network.subscribe and network.unsubscribe methods

Refactor Room Management

cb93487

improve test coverage

0522e15

ballinette added 5 commits August 29, 2017 19:03

add options parameter to Room.subscribe

c14f583

Apply remarks from @scottinet

95f42df

+ add 'use strict' in pure ES6 classes

Allow Kuzzle.query to be called with empty query argument

b28bd16

to enable calls like `kuzzle.queryPromise({controller: 'server', action: 'healthCheck'}).then(...);`

Allow Collection.publishMessage method to be promisified

a6087f3

add options parameter to Room.unsubscribe

dab1ecc

ballinette force-pushed the refactor-network-wrappers branch from 4ff1148 to dab1ecc Compare August 29, 2017 17:03

scottinet approved these changes Sep 12, 2017

View reviewed changes

xbill82 assigned ballinette Sep 12, 2017

xbill82 reviewed Sep 12, 2017

View reviewed changes

dbengsch approved these changes Sep 12, 2017

View reviewed changes

xbill82 approved these changes Sep 12, 2017

View reviewed changes

Ballinette added 2 commits September 14, 2017 18:54

rename wrappers => protocols

55f9dd6

fix remarks from @dbengsch

7b767e7

ballinette force-pushed the refactor-network-wrappers branch from ea2e97d to 7b767e7 Compare September 14, 2017 16:55

Merge branch '6.x' into refactor-network-wrappers

9b81c95

# Conflicts: # src/Kuzzle.js # test/network/networkWrapper.test.js

ballinette merged commit 64fc34c into 6.x Sep 29, 2017

ballinette deleted the refactor-network-wrappers branch October 5, 2017 13:38

This was referenced Nov 10, 2017

Document 6.x SDK javascript kuzzleio/documentation-old#403

Merged

Make backoffice compatible with new SDK 6.0 kuzzleio/kuzzle-admin-console#363

Closed

alexandrebouthinon mentioned this pull request Feb 21, 2019

6.0.0 proposal #377

Merged


		this.kuzzle.callbackRequired('Collection.scrollSpecifications', cb);

		if (options && options.scroll) {


		request.scrollId = scrollId;

		if (options && options.scroll) {

Refactor network wrappers #243

Refactor network wrappers #243

Uh oh!

Conversation

ballinette commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Improvements

Boyscout improvements

Breaking changes

Renamed properties and method in Kuzzle prototype

Removal of SubscribeResult prototype:

Changed signature for Collection.room method and Room constructor

Changed signature for Room.renew method

New option protocol to Kuzzle constructor

Uh oh!

codecov-io commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ballinette Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ballinette Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ballinette Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottinet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xbill82 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ballinette commented Aug 11, 2017 •

edited

Loading

Renamed properties and method in `Kuzzle` prototype

Removal of `SubscribeResult` prototype:

Changed signature for `Collection.room` method and `Room` constructor

Changed signature for `Room.renew` method

New option `protocol` to Kuzzle constructor

codecov-io commented Aug 11, 2017 •

edited

Loading

ballinette Aug 11, 2017 •

edited

Loading

ballinette Aug 11, 2017 •

edited

Loading

ballinette Aug 18, 2017 •

edited

Loading