Add support for an afterBoot callback, when specified

[shlomiassaf]

Hi,

When executing the boot process, async boot modules can not notify when they are done.
Such notification is important in several situations.

For example, we want to register an API endpoint in a boot-module but the registration depends on an async operation (e.g: build a models from a database schema).
We must make sure we register the 'catch-all' route after our boot-module finish to register API endpoints.
Another example is to execute the loopback-explorer after at that point.

This pull implements a basic interface to handle such cases, it does that without changing the current interface and it is transparent to old configurations.

To include a final callback add a afterBoot property to config object that holds a ref to a function.
boot(app, {appRootDir: __dirname, afterBoot: function() {console.log('done'); } } );

By default, a boot-module does not suspend the queue.
For a boot-module to suspend the execution until a boot-module has finished, a boot-module must:

1. Hold a property called waitForCallback with the value true;
2. register a 2nd parameter in its signature, this is the callback parameter.
3. execute the callback when done, in all situations.

Example:

module.exports = waitableBootModule;

function waitableBootModule(server, doneCb) {
    // wait 3 seconds, then continute.
    setTimeout(function () {
        if (doneCb && typeof doneCb == 'function'){
            doneCb();
        }
    }, 3000);
}

waitableBootModule.waitForCallback = true;

It is important to note that afterBoot callback might execute before all boot-modules finish, this can happen when boot-modules without waitForCallback implements async logic.

[slnode]

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

[bajtos]

Hi @shlomiassaf, thank you for taking the time to submit a pull request.

First of all, to solve the use case of adding models dynamically, one should not need to use async boot scripts, both the rest adapter/handler and the loopback-explorer module should update the routing metadata when a new model is added - see strongloop/loopback-component-explorer#22 and strongloop/loopback#193.

Setting that aside, async boot scripts are still a nice thing to have.

I have a problem with the proposed API, as it is not very consistent with the existing approaches as seen in other Node.js modules. Here are better options:

// Option A - export a function with a cb parameter
// loopback-boot can detect number of arguments to decide whether the function is sync/async
// This is similar to how expressjs distinguish between regular and error middleware
module.exports = function(app, cb) {
  // ...
  cb();
}

// Option B - provide `this.async()` to switch from sync to async mode
// This is similar to what Grunt provide
module.exports = function(app) {
  var done = this.async();
  // ...
  done();
};

Since this is an important API-related decision, I'd like to know the opinion of @ritch @raymondfeng @fabien too.

[bajtos]

The same objection applies to afterBoot. IMHO a better solution is to add another (optional) parameter to the boot function.

boot(app, __dirname, cb);

[shlomiassaf]

Hi,

Thank you for the notes.

I would go with option A, its looks more natural to me.

As for the boot function, you suggest adding a 3rd parameter to support both config option approach and string approach, I also think its a better implementation.

I also think afterBoot is not a proper name, if anyone has a different name please change.

I'm working with nodejs for 3 months, still learning conventions.

[bajtos]

I would go with option A, its looks more natural to me.

Cool. Here is the code to get a number of arguments of a function fn: fn.length.

I also think afterBoot is not a proper name, if anyone has a different name please change.

This will be the third argument to boot, right? In which case the convention is to call the argument callback or cb.

I'm working with nodejs for 3 months, still learning conventions.

No worries. I'll keep it in mind and make my comments more descriptive & educational.

[raymondfeng]

I think we should support the following:

• module.exports = function(app, cb) // for the boot script
• module.exports = function(MyModel, cb) // for the model extension js
• boot(app, __dirname, cb) // for boot

We should also decide/document if the boot scripts/model scripts will be run in serial or parallel.

[bajtos]

Let's keep this pull request simple and leave module.exports = function(MyModel, cb) out of scope for now.

As for boot script ordering, I am for serial execution:

• It is the current behaviour, we should preserve backwards compatibility.
• A boot scripts can depend on a result of another boot script. Parallel execution would make this more difficult to implement.

[shlomiassaf]

When I implemented this I thought about parallel execution but I didnt fins it useful.
The documentation states that the order of scripts is by filename, developers take that into consideration and changing it might break their code/logic.

Also, I dont see any need for it, performance wise. Its a piece of code that runs once when the server starts. Clarity, simplicity and clear logic is more important.

[shlomiassaf]

I`ll be going on a vacation this Sunday for about a month (honeymoon) so it might take some time to finish.

If anyone want to help, feel free :)

[bajtos]

@shlomiassaf I`ll be going on a vacation this Sunday for about a month (honeymoon) so it might take some time to finish.

Wow, congratulations on getting married and have a great holiday!

[raymondfeng]

I'm starting to add a more complete implementation driven by the use case to use data source discovery to expose models to REST. See 02b5326. I'll add some tests and submit a PR.

[raymondfeng]

It should be superseded by #50 now.