Closed pierissimo closed 7 years ago
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."
Can one of the admins verify this patch?
Can one of the admins verify this patch?
Can one of the admins verify this patch?
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
@slnode ok to test
@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?
@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:
In this way, the users should have to be aware only of the loopback-primus component, and not of the low-level adapter implementation.
+1 to create a separate component.
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 👍
@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 ?
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.
@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?
@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?
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.
I created a PR based on this discussion: https://github.com/strongloop/strong-remoting/pull/408
I created a PR based on this discussion: #408
Lovely. Can we close this PR #405 then?
Yes, closing.
Description
Primus adapter for strong-remoting
Related issues
215
Checklist