Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added an autostart flag to defer loading #38

Merged
merged 1 commit into from
Dec 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ async function third (instance, opts) {
* <a href="#use"><code>instance.<b>use()</b></code></a>
* <a href="#after"><code>instance.<b>after()</b></code></a>
* <a href="#ready"><code>instance.<b>ready()</b></code></a>
* <a href="#start"><code>instance.<b>start()</b></code></a>
* <a href="#override"><code>instance.<b>override()</b></code></a>
* <a href="#onClose"><code>instance.<b>onClose()</b></code></a>
* <a href="#close"><code>instance.<b>close()</b></code></a>
Expand All @@ -81,7 +82,7 @@ async function third (instance, opts) {
-------------------------------------------------------
<a name="constructor"></a>

### avvio([instance], [started])
### avvio([instance], [options], [started])

Starts the avvio sequence.
As the name suggest, `instance` is the object representing your application.
Expand All @@ -107,6 +108,9 @@ server.use(function first (s, opts, cb) {
Options:

* `expose`: a key/value property to change how `use`, `after` and `ready` are exposed.
* `autostart`: do not start loading plugins automatically, but wait for
a call to [`.start()`](#start)  or [`.ready()`](#ready).


Events:

Expand Down Expand Up @@ -261,6 +265,16 @@ app.ready(function (err, context, done) {

Returns the instance on which `ready` is called, to support a chainable API.

If `autostart: false` is passed as an option, calling `.ready()`  will
also start the boot sequence.

-------------------------------------------------------
<a name="start"></a>

### app.start()

Start the boot sequence, if it was not started yet.

-------------------------------------------------------
<a name="express"></a>

Expand Down
57 changes: 32 additions & 25 deletions boot.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ function Boot (server, opts, done) {
return instance
}

if (opts.autostart !== false) {
opts.autostart = true
}

server = server || this

this._server = server
Expand All @@ -98,6 +102,7 @@ function Boot (server, opts, done) {
this.once('start', done)
}

this.started = false
this.booted = false

this._readyQ = fastq(this, callWithCbOrNextTick, 1)
Expand All @@ -114,34 +119,34 @@ function Boot (server, opts, done) {
}
this._thereIsCloseCb = false

// we init, because we need to emit "start" if no use is called
this._init()
this._doStart = null
const main = new Plugin(this, (s, opts, done) => {
this._doStart = done
if (opts.autostart) {
this.start()
}
}, opts, noop)

Plugin.loadPlugin.call(this, main, (err) => {
debug('root plugin ready')
if (err) {
this._error = err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless this._doStart is called with an error argument this is unreachable. I found this while working on a patch to increase test coverage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what happened, my code selection got moved up a couple lines. should have selected the lines within if (err) {...}.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. Here we receive the errors of all the loaded plugins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to get an error to reach this block, I was unsuccessful so I analyzed the code. I'm confident that it's unreachable but that's not important to this review.

if (this._readyQ.length() === 0) {
throw err
}
} else {
this._readyQ.resume()
}
})
}

inherits(Boot, EE)

Boot.prototype._init = function () {
if (this.booted) {
throw new Error('root plugin has already booted')
}
Boot.prototype.start = function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be added to wrap?

Also if this.started is already true do we want to run this._doStart again?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we should add it to wrap? Why would you want to do it?

An avvio instance can be booted only once. Otherwise too many edge cases happens (this is discussed elsewhere).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind I looked at the docs again it explicitly states which functions are exposed. It needs to be updated to list close and onClose but that's not related to this review. I'll submit a doc update for that later.

this.started = true

if (this._current.length === 0) {
const main = new Plugin(this, function root (s, opts, done) {
// we need to wait any call to use() to happen
process.nextTick(done)
}, {}, noop)
Plugin.loadPlugin.call(this, main, (err) => {
debug('root plugin ready')
if (err) {
this._error = err
if (this._readyQ.length() === 0) {
throw err
}
} else {
this._readyQ.resume()
}
})
}
// we need to wait any call to use() to happen
process.nextTick(this._doStart)
}

// allows to override the instance of a server, given a plugin
Expand Down Expand Up @@ -176,8 +181,9 @@ Boot.prototype._addPlugin = function (plugin, opts, callback) {
opts = opts || {}
callback = callback || null

// we reinit, if use is called after emitting start once
this._init()
if (this.booted) {
throw new Error('root plugin has already booted')
}

// we always add plugins to load at the current element
const current = this._current[0]
Expand Down Expand Up @@ -230,6 +236,7 @@ Boot.prototype.close = function (func) {

Boot.prototype.ready = function (func) {
this._readyQ.push(func)
this.start()
return this
}

Expand Down
50 changes: 50 additions & 0 deletions test/basic.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,53 @@ test('promise long resolve', (t) => {
t.notOk(err)
})
})

test('do not autostart', (t) => {
const app = boot(null, {
autostart: false
})
app.on('start', () => {
t.fail()
})
t.end()
})

test('start with ready', (t) => {
t.plan(2)

const app = boot(null, {
autostart: false
})

app.on('start', () => {
t.pass()
})

app.ready(function (err) {
t.error(err)
})
})

test('load a plugin after start()', (t) => {
t.plan(1)

var startCalled = false
const app = boot(null, {
autostart: false
})

app.use((s, opts, done) => {
t.ok(startCalled)
done()
})

// we use a timer because
// it is more reliable than
// nextTick and setImmediate
// this almost always will come
// after those are completed
setTimeout(() => {
app.start()
startCalled = true
}, 2)
})