Feature/primus adapter

[pierissimo]

Description

Primus adapter for strong-remoting

Related issues

• Primus Adapter #215

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[bajtos]

Hi @pierissimo, thank you for the pull request. Before I get to detailed review, please sign our CLA here: https://cla.strongloop.com/agreements/strongloop/strong-remoting

[bajtos]

@slnode ok to test

[bajtos]

@pierissimo I took a quick look at your proposed changes, they look reasonable to me.

At high level, we would prefer to move individual adapters (REST, WS) outside of strong-remoting core, see #141. Have you considered implementing the adapter as a standalone npm module?

// expose it over http
var server =
    require('http')
        .createServer()
        .listen(9000);

remotes.handler('primus', server);

I think this makes it difficult to use Primus (but also the existing WebSocket adapter) as a regular transport in LoopBack apps, where users are expected to do:

var app = loopback();
app.use(loopback.rest());
app.listen();

I think that's another argument for making the Primus adapter a LoopBack component instead of a strong-remoting adapter. I am envisioning instructions like the following:

const loopbackPrimus = require('loopback-primus');
var app = loopback();
loopbackPrimus(app, {/* config */});
app.listen();
// ^-- should emit an event that loopbackPrimus can use
// to hook on the `server` object

Thoughts?

@raymondfeng @ritch what's your opinion?

[pierissimo]

@bajtos
The ability to dynamically provide an adapter would be a great addition.
The part of the code involved could just be that one.
The function should also ensure that the adapter provided implements some required methods.

Not sure how a loopback-primus component could work without a primus adapter implementation.

To summarise, a solution for me would be:

• Implement a mechanism allowing strong-remoting users to provide their own transport/adapter implementations.
• Implement a loopback-component that has the responsibility to provide a primus adapter implementation.

In this way, the users should have to be aware only of the loopback-primus component, and not of the low-level adapter implementation.

[raymondfeng]

+1 to create a separate component.

[bajtos]

Not sure how a loopback-primus component could work without a primus adapter implementation.

Take a look at how RemoteObjects.handler() are implemented here:

RemoteObjects.prototype.handler = function(name, options) {
  var Adapter = this.adapter(name);
  var adapter = new Adapter(this, options);
  var handler = adapter.createHandler();

  if (handler) {
    // allow adapter reference from handler
    handler.adapter = adapter;
  }

  return handler;
};

In essence, we start with a RemoteObjects instance and end up with a handler function that can be mounted as Express middleware. I believe the same can be achieved via a LoopBack component too:

const PrimusAdapter = require('./primus-adapter');

module.exports = setupPrimusComponent(app, options) {
  const remoteObjects = app.remotes();

  // I'm using Adapter pattern to make this easier to understand.
  // Once we de-couple from strong-remoting,
  // this code can become anything
  const adapter = new PrimusAdapter(remoteObjects, options);
  const handler = adapter.createHandler(); // is this needed at all?

  app.on('server', server => /* initialize Primus on `server */);
};

It's entirely possible I am missing something important in my proposal, let's discuss!

• Implement a mechanism allowing strong-remoting users to provide their own transport/adapter implementations.
• Implement a loopback-component that has the responsibility to provide a primus adapter implementation.

Yes, that's what I had in mind in #141 👍

[pierissimo]

@bajtos
Ok make sense.
The new .handler function could look like:

RemoteObjects.prototype.handler = function(name, options, ProvidedAdapterClass) {
  var Adapter = this.adapter(name, ProvidedAdapterClass);
  var adapter = new Adapter(this, options);
  var handler = adapter.createHandler();

  if (handler) {
    // allow adapter reference from handler
    handler.adapter = adapter;
  }

  return handler;
};

the .adapter function should implement a method that check if ProvidedAdapterClass satisfy the minimum requirements:

RemoteObjects.prototype.adapter = function(name, AdapterClass) {
  if (AdapterClass) {
    this.checkAdapterRequirements(AdapterClass); //throws error
    return AdapterClass;
  } else {
    return require('./' + name + '-adapter');
  }
};

Would it be a good start for the implementation described in #141 ?

[bajtos]

Hey, sorry for the delay. I was on vacation for few days and now I have too many notifications to process. I'll try to respond here by the end of the next week.

[bajtos]

@pierissimo your proposal sounds reasonable to me. Considering that name and AdapterClass are mutually exclusive, I am proposing a slightly different API:

RemoteObjects.prototype.handler = function(nameOrClass, options) {
  var Adapter = this.adapter(nameOrClass);
  var adapter = new Adapter(this, options);
  var handler = adapter.createHandler();

  if (handler) {
    // allow adapter reference from handler
    handler.adapter = adapter;
  }
  // ^-- BTW, can we move this code into adapter.createHandler()?

  return handler;
};

RemoteObjects.prototype.adapter = function(nameOrClass) {
  if (typeof nameOrClass === 'function') {
    this.checkAdapterRequirements(nameOrClass); //throws error
    return nameOrClass;
  } else {
    return require('./' + nameOrClass + '-adapter');
  }
};

Thoughts?

[pierissimo]

@bajtos ok makes sense.
The user will be responsible to provide all the necessary logic for the correct functioning of the adapter (the context, for example).
As far I can see the only required method for an adapter is 'createHandler'.
Is that correct?

[bajtos]

As far I can see the only required method for an adapter is 'createHandler'.
Is that correct?

That's needed on the server.

If you want to use the adapter in a client (to make outgoing requests to the server using the same adapter), then I think you need to implement prototype.connect and prototype.invoke too.

[pierissimo]

I created a PR based on this discussion:
#408

[bajtos]

I created a PR based on this discussion: #408

Lovely. Can we close this PR #405 then?

[pierissimo]

Yes, closing.