lib: prepare code for metarhia-common and metasync #246

tshemsedinov · 2017-06-28T21:46:20Z

@aqrln This PR is not complete, just for your interest. I'll do it in multiple steps and I may need your help.

tshemsedinov · 2017-07-18T01:48:13Z

@aqrln done, but I rebased this branch with branch style-and-opt to avoid conflicts with eb7a704 lib: refactor return style so PR includes commit from different PR.

tshemsedinov · 2017-07-22T01:36:52Z

@aqrln rebased again

aqrln · 2017-07-22T01:55:21Z

lib/applications.js

@@ -21,24 +23,25 @@ class Application {
  //   callback - method callback
  //
  callMethod(connection, interfaceName, methodName, args, callback) {
+    const cb = common.cb(callback);


I don't think we need it here. Connection#callMethod() takes care of optional callbacks. This is an internal method and we are pretty safe making the last argument required.

aqrln · 2017-07-22T01:56:33Z

lib/connection.js

-      if (callback) {
-        callback(error, sessionId);
-      }
+      callback(error, sessionId);


What is the reason of this change?

aqrln · 2017-07-22T02:00:25Z

lib/remote-proxy.js

-  }
-
-  this._connection.callMethod(this._interfaceName, methodName, args, callback);
+  const cb = common.cbExtract(args);


Can we please call it callback? I'd prefer to use descriptive names when possible.

aqrln · 2017-07-22T02:01:19Z

lib/socket.js

@@ -83,12 +85,12 @@ class Transport extends EventEmitter {
 }

 const socketFactory = connect => (...options) => {
-  const callback = common.extractCallback(options);
+  const cb = common.cbExtract(options);


aqrln · 2017-07-22T02:01:31Z

lib/transport-common.js

@@ -27,10 +29,10 @@ const common = require('./common');
 const newConnectFn = (
  connFactory, transportClass
 ) => (appName, client, ...options) => {
-  const callback = common.extractCallback(options);
+  const cb = common.cbExtract(options);


aqrln · 2017-07-22T02:04:00Z

lib/transport-common.js

-      let errored = false;
-      const len = interfaces.length;
-      let count = 0;
+      const pCollector = metasync.collect(interfaces.length);


Maybe something like proxiesCollector or even just collector. That lone "p" letter doesn't help readability much and looks like hungarian notation.

aqrln · 2017-07-22T02:06:06Z

lib/ws-browser.js

@@ -80,14 +82,15 @@ class Transport extends EventEmitter {
 //   appProvider - client application provider
 //
 const socketFactory = (url, callback) => {
+  const cb = common.cb(callback);


I'm not sure whether we need it here, it may be handled at a higher level.

/cc @lundibundi

This is also an internal function, there is always a callback here.

aqrln · 2017-07-22T02:08:01Z

lib/ws-internal.js

@@ -124,13 +126,13 @@ const initServer = function(options, httpServer) {
 //             is established.
 //
 const connectionFactory = (webSocketConfig, ...options) => {
-  const callback = common.extractCallback(options);
+  const cb = common.cbExtract(options);


I'd prefer it to be callback.

aqrln · 2017-07-22T02:10:07Z

package-lock.json

-      "version": "4.11.8",
-      "resolved": "https://registry.npmjs.org/ajv/-/ajv-4.11.8.tgz",
-      "integrity": "sha1-gv+wKynmYq5TvcIK8VlHcGc5xTY=",
+      "version": "5.2.2",


There's a huge deal of unrelated changes and version bumps. Can you please revert all the changes to package.json and package-lock.json and run npm i metasync metarhia-common anew?

@aqrln but this PR requires metasync and metarhia-common on package.json

Yes. But running npm install metasync metarhia-common will add them to package.json too.

aqrln · 2017-07-22T02:11:40Z

tools/common.js

+
+const writeFile = promisify(fs.writeFile);
+
+module.exports = {


This change is unrelated. Can you please split it into another PR?

tshemsedinov · 2017-07-24T13:31:12Z

@aqrln see dff1d34 it is related to this PR because of naming but extracted to separate commit.

tshemsedinov · 2017-08-01T17:55:45Z

@aqrln rebased and ready

belochub · 2017-08-02T12:43:58Z

lib/cli/line-processor.js

@@ -75,13 +75,13 @@ module.exports = class LineProcessor {
      callback(reportMissingArgument('Host'));
      return;
    }
-    const args = utils.split(tokens, ' ', 2);
-    let [scheme, authority] = utils.split(args[0], '://', 1, true);
+    const args = autocomplete.split(tokens, ' ', 2);


split function here has nothing to do with autocomplete whatsoever, it is just used to split up the arguments provided by the user in CLI.

I agree, but see https://github.com/metarhia/jstp/pull/246/files#r132857348 We must arrange such common utilities to have code consistency over project, Meterhia stack and JS stories in general.

nechaido · 2017-08-02T12:48:00Z

package.json

@@ -37,6 +37,8 @@
  "dependencies": {
    "semver": "^5.4.1",
    "uuid": "^3.1.0",
+    "metarhia-common": "0.0.19",


Shouldn't it be: "metarhia-common": "^0.0.19",?

@nechaido both 0.0.19 and ^0.0.19 mean the same thing. Caret ranges only allow updates that don't modify the leftmost non-zero part of the version.

nechaido · 2017-08-02T15:06:12Z

lib/cli/autocomplete.js

+
+module.exports = {
+  filterKeys,
+  split,


Why have you moved rsplit to metarhia-common but have not moved split?

I changed rsplit to common.rsection because it is not compatible with String.prototype.split() This split is also not compatible with String.prototype.split() so I think we will either rename it or use String.prototype.split() instead.

lundibundi · 2017-08-03T09:56:33Z

lib/socket.js

@@ -84,7 +85,7 @@ class Transport extends EventEmitter {

 const socketFactory = (connect) => {
  const resultConnectFn = (...options) => {
-    const callback = common.extractCallback(options);
+    const callback = common.cbExtract(options);


extractCallback and cbExtract are not equivalent, the latter has a lot of checks that are completely unnecessary here. I think it's better to place extractCallback in common and rename cbExtract to something that will represent its purpose, like cbWrapExtract.

Yes, I agree, in metarhia-common v0.0.21 we have safeCallback (renamed cbExtract) and unsafeCallback (renamed cbUnsafe). I placed your code of extractCallback into common.unsafeCallback because it's optimal implementation. Also I renamed common.cb to once because once will be clear and native for JS world.

In this case user can call it without callback so we need common.safeCallback

This is an internal function that always has callback, the user cannot call it.

This callback comes from jstp.net.connect and jstp.net.connectAndInspect and tls/ws/wss analogs. Try to remove this callack in test/node/transport-net-tcp.js for example.

At the time it throws a TypeError:

> jstp.net.connect('name', null, 123); undefined > TypeError: callback is not a function at connFactory (/home/nechaido/Projects/metarhia/jstp/lib/transport-common.js:35:7) at Socket.onceErrorListener (/home/nechaido/Projects/metarhia/jstp/lib/socket.js:91:9) at Object.onceWrapper (events.js:316:30) at emitOne (events.js:115:13) at Socket.emit (events.js:210:7) at emitErrorNT (internal/streams/destroy.js:64:8) at _combinedTickCallback (internal/process/next_tick.js:138:11) at process._tickDomainCallback (internal/process/next_tick.js:218:9)

I am ok with it. connectis not a function that can be called without callback.

We need to avoid throwing, it terminates process, it's better to bubble an error to upper level without process termination.

But I can agree that connect is client-side function, client-side code is less durable in termination/restarting meaning.

@tshemsedinov, but it's a completely natural behavior in Node.js for TypeError to occur whenever you provide invalid arguments to the function (which is the case with providing no callback to the connect function).
This means that the user gets TypeError thrown only when he/she is using the library incorrectly.

lundibundi · 2017-08-03T09:57:49Z

lib/transport-common.js

@@ -29,7 +31,7 @@ const common = require('./common');
 const newConnectFn = (
  connFactory, transportClass
 ) => (app, client, ...options) => {
-  const callback = common.extractCallback(options);
+  const callback = common.cbExtract(options);


Same as cbExtract comment.

https://github.com/metarhia/jstp/pull/246/files#r132848995

In this case user can call it without callback so we need common.safeCallback

In this case not providing a callback is both an error and strange (you are calling a function to create a connection and not expecting a result), that's why I used extract here.

lundibundi · 2017-08-03T09:57:54Z

lib/transport-common.js

@@ -73,30 +75,21 @@ const newConnectFn = (
 const newConnectAndInspectFn = (connFactory, transportClass) => {
  const connect = newConnectFn(connFactory, transportClass);
  return (app, client, interfaces, ...options) => {
-    const callback = common.extractCallback(options);
+    const callback = common.cbExtract(options);


Same as cbExtract comment.

https://github.com/metarhia/jstp/pull/246/files#r132848995

In this case user can call it without callback so we need common.safeCallback

Same as #246 (comment).

lundibundi · 2017-08-03T09:59:13Z

lib/ws-internal.js

@@ -124,7 +125,7 @@ const initServer = function(options, httpServer) {
 //             is established.
 //
 const connectionFactory = (webSocketConfig, ...options) => {
-  const callback = common.extractCallback(options);
+  const callback = common.cbExtract(options);


Same as cbExtract comment.

https://github.com/metarhia/jstp/pull/246/files#r132848995

In this case user can call it without callback so we need common.safeCallback

Same as #246 (comment).

lundibundi · 2017-08-03T10:10:00Z

lib/applications.js

@@ -17,7 +18,7 @@ const errors = require('./errors');
 //
 class Application {
  constructor(name, api, version) {
-    [this.name, this.version] = common.rsplit(name, '@');
+    [this.name, this.version] = common.rsection(name, '@');


I think that it's better to update rsplit function to support cases from python where I actually took it from and name it as such.

I agree, we need full rsplit so I added issue here: metarhia/common#91 but I think in this place rsection is ok because it is simpler and in multiple other cases rsection will be optimal.

tshemsedinov · 2017-08-14T01:05:28Z

In most cases we use extractCallback (new name unnsafeCallback) to get callback from user-defined connection function, so user can pass callback optionally. If no callback passed exception will throw. In such cases we may choose one of the following ways:

Use unsafeCallback and declare callback as required, so user will be responsible for inappropriate use.
Implement requiredCallback that requires last argument to be callback and throw exception otherwise.
Use safeCallback and have no problems but overhead.
Change safeCallback behavior: use empty function but remove prevention of multiple calls, so it will be safe enough for above this cases. For more strict cases we can implement onceCallback.

I'd prefer to have in metarhia-common all those cases:

unsafeCallback(array): function or null
safeCallback(array): function or emptiness
requiredCallback(array): function or throw
onceCallback(array): once(function) or emptiness

@metarhia/jstp-core

belochub · 2018-07-10T21:01:17Z

Closing this PR, since it's become outdated to the point where it's easier to rewrite it from scratch compared to resolving all of the merge conflicts when rebasing on master.
I will also create a corresponding issue to keep as a reminder (as proposed by @tshemsedinov).

tshemsedinov requested a review from aqrln June 28, 2017 21:46

tshemsedinov added the lib label Jun 29, 2017

belochub added the work-in-progress label Jul 6, 2017

tshemsedinov force-pushed the use-metarhia-common branch from 84d734e to 880f6d1 Compare July 9, 2017 21:55

tshemsedinov force-pushed the use-metarhia-common branch from 880f6d1 to b8ec94c Compare July 18, 2017 01:41

tshemsedinov changed the title ~~lib: prepare code to use metarhia-common~~ lib: prepare code for metarhia-common and metasync Jul 18, 2017

tshemsedinov removed the work-in-progress label Jul 18, 2017

tshemsedinov force-pushed the use-metarhia-common branch from b8ec94c to 7d3cd28 Compare July 22, 2017 01:36

aqrln reviewed Jul 22, 2017

View reviewed changes

metarhia deleted a comment from lundibundi Jul 22, 2017

tshemsedinov force-pushed the use-metarhia-common branch 5 times, most recently from ce962f5 to 6978e61 Compare July 22, 2017 12:40

tshemsedinov added the work-in-progress label Jul 29, 2017

tshemsedinov force-pushed the use-metarhia-common branch 2 times, most recently from f59195d to ad69af0 Compare August 1, 2017 17:53

tshemsedinov removed the work-in-progress label Aug 1, 2017

belochub reviewed Aug 2, 2017

View reviewed changes

nechaido reviewed Aug 2, 2017

View reviewed changes

lundibundi reviewed Aug 3, 2017

View reviewed changes

lib: prepare code for metarhia-common and metasync

ff0d1af

tshemsedinov force-pushed the use-metarhia-common branch from ad69af0 to cb9cbbe Compare August 14, 2017 00:38

fixup! lib: prepare code for metarhia-common and metasync

a9e645c

tshemsedinov force-pushed the use-metarhia-common branch from cb9cbbe to a9e645c Compare August 14, 2017 00:40

belochub closed this Jul 10, 2018

belochub mentioned this pull request Jul 10, 2018

Prepare code for metarhia-common and metasync #344

Open


		const writeFile = promisify(fs.writeFile);

		module.exports = {

lib: prepare code for metarhia-common and metasync #246

lib: prepare code for metarhia-common and metasync #246

Conversation

tshemsedinov commented Jun 28, 2017

tshemsedinov commented Jul 18, 2017

tshemsedinov commented Jul 22, 2017

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

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

Choose a reason for hiding this comment

tshemsedinov commented Jul 24, 2017

tshemsedinov commented Aug 1, 2017

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

Choose a reason for hiding this comment

tshemsedinov Aug 13, 2017 • 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

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

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

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

tshemsedinov commented Aug 14, 2017

belochub commented Jul 10, 2018

tshemsedinov Aug 13, 2017 •

edited

Loading