outmoded / university

Community learning experiment
Other
371 stars 193 forks source link

Discrepancies in registering plugins #218

Closed garfbargle closed 8 years ago

garfbargle commented 8 years ago

In /lib/start.js, our manifest sets us up and we register Inert and Vision.

In /lib/home.js, we set server dependencies for Inert and Vision. We wait for them before setting up the home plugin.

However, in /lib/lout.js, we re-register Inert and Vision along with the 'Lout' plugin.

Why does /lib/home.js not register the plugins instead? Why is there a difference between //lib/home.js and /lib/lout.js?

zoe-1 commented 8 years ago

@garfbargle good observation. Will look into it and write a response.

zoe-1 commented 8 years ago

@garfbargle cleaned up /lib/lout.js to follow similar logic as /lib/home.js and other plugins. The project uses Glue to register plugins so registering inert and vision in the lout plugin or other plugin is not good style. Instead plugins that have dependencies like inert or vision use dependency.after logic to ensure dependencies are loaded first. Thanks for pointing out the issue.

garfbargle commented 8 years ago

@zoe-1 Awesome, that's what I was thinking should be the right approach after studying the project. Thanks for clearing that up!

ferrao commented 7 years ago

@zoe-1 I belive you did something wrong with this:

server.dependency(['inert', 'vision'], internals.after(server, next));

as you seem to be calling internals.after() immediately. I belive it should be:

server.dependency(['inert', 'vision'], internals.after);
return next();

But then i can not escape this:

Error: Cannot add onPreStart (after) extension after the server was initialized

zoe-1 commented 7 years ago

@ferrao I have not seen your code, but most likely you are describing an asynchronous issue.
In short, internals.after() is not executed immediately. server.dependency(['dependencies'], after) first registers dependencies, then the after function is executed "after" dependencies are registered.

Plugins are loaded at the initialization stage of a hapi server start up. The next() execution tells hapi that the plugin registration completed causing hapi to move on to other registrations. But, in your code, next() is called before the completion of the internals.after() registration logic and you get the error message. This happens because of JavaScripts' asynchronous execution of code. JavaScript executes return next() before your server.dependency() completes.

See ./lib/lout.js in master branch of university repo for example.

server.dependency(['inert', 'vision'], internals.after(server, options, next));
return next();  // incorrect use of next();

If we did the above in ./lib/lout.js, the registration of ./lib/lout.js would exit prematurely (before the server.dependency() completes). However, if we drop return next() as below, next() would execute at the completion of registering the lout plugin. Notice below: next() is passed to the internals.after() to be executed later.
Correct example below.

server.dependency(['inert', 'vision'], internals.after(server, options, next));

Usually, the error message is:

Error: Cannot start server while it is in initializing state

Relevant Documentation: plugin tutorial dependencies after docs

ferrao commented 7 years ago

@zoe-1 both ./lib/lout.js and your example

server.dependency(['inert', 'vision'], internals.after(server, options, next));

are not correct. The after argument needs to be a function and not a function invocation.

Look at the docs example:

const after = function (server, next) {
    // Additional plugin registration logic
    return next();
};

exports.register = function (server, options, next) {
    server.dependency('yar', after);
    return next();
};

It is quite clear for me that the code is wrong as it is, but i am lost as to what is the proper way to set this up using server.dependecy(). I now that i can avoid it by declaring decencies in attributes, but sometimes we need to do additional stuff after dependencies are registered.

zoe-1 commented 7 years ago

You are right. made corrections with #235 and #234.