Allow to specify custom server instance in options

[walling]

This pull request adds option.server, which accepts a custom server instance to be used. The manifest.server is not allowed to be specified along with option.server.

My use case for creating this pull request:

1. I want to be able to specify the exact version of Hapi in my package.json. I can do this now and pass the server instance to Glue.
2. I want to export my server instance for unit tests synchronously, before Glue has done its work. I can do this by creating the server instance, export it, and pass it to Glue to do the magic.

Any comments/feedback is appreciated.

[walling]

Btw. the reason that the Travis CI build failed is because of 3 linting errors that already were there. I added two tests and they pass successfully.

[AdriVanHoudt]

so this would also allow to do some config on the server yourself and then pass it to glue to do the rest?

[walling]

@AdriVanHoudt, yes, no problem. I haven't made a unit test for it though.

[AdriVanHoudt]

@walling yeah I would definitely test with different configs and pre built servers, I can imagine stuff going wrong when you configure something and then Glue wants to override or something

[walling]

My personal use case is to pass an un-cofigured server instance, but I get it. I'll look into it soonish. How throughly do you think it should be? I'm sure not sure it's possible to test for all failure cases, but I'll look at the cases where Glue might override something.

[AdriVanHoudt]

Yeah a basic one and a test where glue would override

[walling]

@AdriVanHoudt, I added some connections and plugins to the existing test, and a new test for accepting a pre-configured server instance. Is it what you had in mind?

[AdriVanHoudt]

@walling almost but glue never overrides the custom server right? Thats a test I would like to see

[walling]

@AdriVanHoudt, can you review the latest test case?

[AdriVanHoudt]

The no empty conn is nice. Can you make a test case were Glue actually does override the custom server?

[walling]

I'm afraid I do not follow you. Can you maybe restate your position or provide a code example?

The tests are considering all cases:

Custom Instance	Glue Manifest	Resulting Server
uninitialised	has config	config from glue
configured	empty	no changes
configured	has config	config is merged

The last test is to ensure that you cannot pass manifest.server when using custom server instance.

[walling]

Ah, sorry, maybe I get it now. I'm a bit slow, I guess. Do you mean, if I specify the same connection in both custom instance and glue manifest? Or if I specify the same plugin in both custom instance and glue manifest?

[AdriVanHoudt]

yeah, I don't really know an example of the top of my head but maybe I should have used conflicting configs where, as you stated, you specify the same plugin or the same connection on the same port or something.

[walling]

Just give me some time, I'll look at it.

[walling]

Now I think we are getting into a bit of troubles. For connections, should it just add them regardless whether they match an existing connection? I'm not sure you can create a good primary key of a connection to actually test whether it was already added ({ port: 1234 } and { address: '127.0.0.1', port: 1234 }) are the same). Take this example:

var server = new Hapi.Server();
server.connection({ port: 8080 });
server.connection({ port: 8081 });

var manifest = {
    connections: [
        { port: 8080 },
        { port: 8081 }
    ]
};

Glue.compose(manifest, { server: server }, ... // 4 connections

I'm not sure how to reasonable handle this case.

Then with plugins, I thought multiple registrations of a plugin is prohibited, but when I test it, Glue just overwrites the helloworld plugin with new options. I have to look into this a bit more.

[AdriVanHoudt]

so adding 2 conns with the same port will just throw I think so it is up to you to decide whether glue or you get the final call also pinging @csrl to join the discussion since he is the maintainer

[walling]

In my opinion the right solution is to make Glue either throw or give error asynchronously. It is a user error to try applying conflicting configs. I don't think Glue should handle it, it should just tell the user that it will fail. The problem right now is, that it is just accepted. It might fail, when you try to start the server. I'm not sure how to properly check for conflicting connections.

[AdriVanHoudt]

I talked to @Marsup and he raised a good point, "when you want to do something custom with your server you should do it in a plugin"

[walling]

Well, I just called it custom server instance. :-) My use case is to be able to manage dependencies, ie. a fixed hapi version and export the instance synchronously in my module. The reason being, I use hapi in a production environment, and I want to be able to specify the version in package.json. I don't want to depend on the version specified in glue's package.json, because it might not be the version that works in our production environment. I just want to minimise the risk that anything can break. I still like to use glue, because of the nice manifest format, which is more readable and maintainable than doing it manually. I will actually use it like this:

var server = module.exports = new Hapi.Server();
Glue.compose(manifest, { server: server, relativeTo: __dirname }, ...);

Then I'm sure that the version of hapi, is the one I specified in my package.json.

I could add in the tests, that the server must be unconfigured, to support my use case. Ie. no connections and no plugins, otherwise glue will just throw an error. Then there are no issues with merging configuration and conflicts. What do you think, @AdriVanHoudt?

[Marsup]

I don't think it's the good solution to that problem. I know peerDependencies are considered bad but that would still be better.

[walling]

@Marsup, what are your proposed solutions concretely? I have two issues:

1. Specify hapi version
2. Export hapi server instance synchronously, so my tests can call server.inject

Unfortunately I don't have a lot of time to discuss. If you have any ideas about how I could improve my architecture, I'm eager to hear it.

[Marsup]

1. Glue would need to change to support any (or a range of) hapi peer dependency.
2. Why synchronous ? This is not necessary nor achievable.

[walling]

Well, regarding (2), we have server.js, it looks like this (simplified version):

var server = module.exports = new Hapi.Server();
module.exports.loaded = when.promise(function(resolve, reject) {
  Glue.compose(manifest, { server: server, relativeTo: __dirname }, function(error, server) {
    // resolve/reject promise
  });
});

A test looks like this:

describe("Some route", function() {

  it("implements some feature", function(done) {
    var options = { ... };
    server.inject(options, function(response) {
      expect(response.statusCode).to.equal(200);
      done();
    });
  });

  // Wait for server to be ready.
  before(function(done) {
    server.loaded.then(done);
  });

});

Is there a better way?

[Marsup]

Not that I know of, why doesn't it work for you ?

[walling]

Sorry, the previous code is the solution I actually want to use, but right now you can't pass a server instance to glue. So we are using this hack instead in server.js:

var loaded = module.exports.loaded = when.promise(function(resolve, reject) {
  Glue.compose(manifest, { relativeTo: __dirname }, function(error, server) {
    // resolve/reject promise
    module.exports = server;
    module.exports.loaded = loaded;
  });
});

It sort of works, because we overwrite the module.exports late, and the tests wait for the loaded event until they use the exported object, but it is really a hack. And it could fail, if something thinks that the server is available and binds the functions before the loaded event.

[walling]

I think we might change back to hapi without glue. After thinking about it, I'm not sure that glue is a good fit for our requirements, and I'm working on this pull request in my limited spare time.

[Marsup]

I don't see any difference, your plugins have a chance to be registered asynchronously anyway, you'll still have to deal with it. Why are you overwriting your exports ? It's not needed.

[walling]

How do I call server.inject if I don't export the server instance somehow?

[Marsup]

Your server is the result of the promise.

[walling]

Yes, tried that, but it leads to ugly code in the tests. Either you need server.loaded.then(function(server) { ... everywhere or in the before task, something like _server.loaded.then(function(_server) { server = _server; ....

No, worries! We are reverting back to just hapi, glue is not a good fit for our requirements. Closing this issue, as I don't have time maintaining it. If anyone wants to pick up the pull request, be my guest.

[Marsup]

As I said you'll have the same problem with bare hapi, plugins may be async. I fail to see how it's ugly, it's javascript as usual.

[csrl]

If the point was to control what version of Hapi, then this isn't the way to do it. Glue specifies the version(s) of hapi it is compatible with in its package dependencies. NPM will resolve that dependency appropriately, so if you have a specific compatible version of Hapi you want to use, then install that version such that NPM can resolve it.

[lock[bot]]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.