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.

Sign up for GitHub

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

After/Ready error handling #13

Merged
merged 6 commits into from
Mar 31, 2017
Merged

After/Ready error handling #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
68a5faf
Updated ready/after to accept errors
delvedor Mar 30, 2017
c9332ef
Updated test
delvedor Mar 30, 2017
6611ca4
Updated docs
delvedor Mar 30, 2017
a0c67bb
Updated docs
delvedor Mar 31, 2017
b5ccb3e
Updated error handling
delvedor Mar 31, 2017
5af6c69
Updated test
delvedor Mar 31, 2017
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
36 changes: 19 additions & 17 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const boot = require('avvio')()

boot
.use(first, { hello: 'world' })
.after((cb) => {
.after((err, cb) => {
console.log('after first and second')
cb()
})
Expand Down Expand Up @@ -103,7 +103,7 @@ server.use(function first (s, opts, cb) {
s.use(function second (s, opts, cb) {
cb()
}, cb)
}).after(function (cb) {
}).after(function (err, cb) {
// after first and second are finished
cb()
})
Expand All @@ -115,7 +115,6 @@ Options:

Events:

* `'error'`  if something bad happens
* `'start'`  when the application starts

The `boot` function can be used also as a
Expand Down Expand Up @@ -148,23 +147,24 @@ The functions will be loaded in the same order as they are inside the array.
-------------------------------------------------------
<a name="after"></a>

### app.after(func([context], [done]), [cb])
### app.after(func(error, [context], [done]), [cb])
Copy link
Member

Choose a reason for hiding this comment

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

I do not think after()  should be called with an error at all. We should bail off the plugin loading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why not, in this way the user can handle the error inside the after.
Example:
If plugin 1 fails => load plugin 2.

Copy link
Member

Choose a reason for hiding this comment

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

that should be in the register callback, not in after IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, but at the moment this is the behaviour:

test('error', (t) => {
  t.plan(2)

  const server = { my: 'server' }
  const app = boot(server)

  app.use(function (s, opts, done) {
    done(new Error('err'))
  }, err => {
    t.ok(err)
    app.use(function (s, opts, done) {
      done()
    })
  })

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

As you can see, we have not that capability, because the error will arrive to the ready callback in every case.
With after I can "block" that error ina specific point, as you can see here:

test('error', (t) => {
  t.plan(3)

  const server = { my: 'server' }
  const app = boot(server)

  app.use(function (s, opts, done) {
    done(new Error('err'))
  }, err => {
    t.ok(err)
  })

  app.after(function (err) {
    t.ok(err)
    app.use(function (s, opts, done) {
      done()
    })
  })

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

This is why yesterday I was saying that we should raise the error in the ready callback only if the error is not handled in the use callback.

Copy link
Member

Choose a reason for hiding this comment

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

OK for this behavior you are describing right know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I didn't understand, do you want me to implement this behaviour?

This is why yesterday I was saying that we should raise the error in the ready callback only if the error is not handled in the use callback.

Copy link
Member

Choose a reason for hiding this comment

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

This is why yesterday I was saying that we should raise the error in the ready callback only if the error is not handled in the use callback.

yes.


Calls a function after all the previously defined plugins are loaded, including
all their dependencies. The `'start'` event is not emitted yet.
If one parameter is given to the callback, that parameter will be the `done` callback.
If two parameters are given to the callback, the first will be the `contenxt`, the second will be the `done` callback.
1. If one parameter is given to the callback, that parameter will be the `error` object.
2. If two parameters are given to the callback, the first will be the `error` object, the second will be the `done` callback.
3. If three parameters are given to the callback, the first will be the `error` object, the second will be the `context` and the third the `done` callback.

```js
const server = {}
...
// after with one parameter
boot.after(function (done) {
// after with two parameter
boot.after(function (err, done) {
done()
})

// after with two parameters
boot.after(function (context, done) {
// after with three parameters
boot.after(function (err, context, done) {
assert.equal(context, server)
done()
})
Expand All @@ -178,23 +178,25 @@ chainable API.
-------------------------------------------------------
<a name="ready"></a>

### app.ready(func([context], [done]))
### app.ready(func(error, [context], [done]))

Calls a functon after all the plugins and `after` call are
completed, but befer `'start'` is emitted. `ready` callbacks are
executed one at a time.
If one parameter is given to the callback, that parameter will be the `done` callback.
If two parameters are given to the callback, the first will be the `contenxt`, the second will be the `done` callback.
1. If one parameter is given to the callback, that parameter will be the `error` object.
2. If two parameters are given to the callback, the first will be the `error` object, the second will be the `done` callback.
3. If three parameters are given to the callback, the first will be the `error` object, the second will be the `context` and the third the `done` callback.

```js
const server = {}
...
// ready with one parameter
boot.ready(function (done) {
// ready with two parameter
boot.ready(function (err, done) {
done()
})

// ready with two parameters
boot.ready(function (context, done) {
// ready with three parameters
boot.ready(function (err, context, done) {
assert.equal(context, server)
done()
})
Expand Down
32 changes: 22 additions & 10 deletions boot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ function Boot (server, opts, done) {

this._server = server
this._current = []
this._error = null

if (done) {
this.once('start', done)
Expand Down Expand Up @@ -93,7 +94,10 @@ Boot.prototype._init = function () {
}, {}, noop)
loadPlugin.call(this, main, (err) => {
if (err) {
this.emit('error', err)
this._error = err
if (this._readyQ.length() === 0) {
throw err
}
} else if (this._readyQ.length() > 0) {
this._readyQ.resume()
} else {
Expand Down Expand Up @@ -133,7 +137,7 @@ Boot.prototype._addPlugin = function (plugin, opts, callback) {
throw new Error('plugin must be a function')
}
opts = opts || {}
callback = callback || noop
callback = callback || null

// we reinit, if use is called after emitting start once
this._init()
Expand All @@ -146,7 +150,7 @@ Boot.prototype._addPlugin = function (plugin, opts, callback) {
// we add the plugin to be loaded at the end of the current queue
current.q.push(obj, (err) => {
if (err) {
this.emit('error', err)
this._error = err
}
})
}
Expand Down Expand Up @@ -193,8 +197,14 @@ Plugin.prototype.exec = function (server, cb) {

Plugin.prototype.finish = function (err, cb) {
const callback = this.callback
callback(err)
cb(err)
// if 'use' has a callback
if (callback) {
callback(err)
// if 'use' has a callback but does not have parameters
cb(callback.length > 0 ? null : err)
} else {
cb(err)
}
}

// loads a plugin
Expand Down Expand Up @@ -222,14 +232,16 @@ function callWithCbOrNextTick (func, cb, context) {
context = this._server
}

if (func.length === 0) {
func()
if (func.length === 0 || func.length === 1) {
func(this._error)
process.nextTick(cb)
} else if (func.length === 1) {
func(cb)
} else if (func.length === 2) {
func(this._error, cb)
} else {
func(context, cb)
func(this._error, context, cb)
}
// with this the error will appear just in the next after/ready callback
this._error = null
}

module.exports = Boot
Expand Down
Loading