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 3 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
23 changes: 13 additions & 10 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,23 +148,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) {
boot.after(function (err, done) {
done()
})

// after with two parameters
boot.after(function (context, done) {
boot.after(function (err, context, done) {
assert.equal(context, server)
done()
})
Expand All @@ -178,23 +179,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) {
boot.ready(function (err, done) {
done()
})

// ready with two parameters
boot.ready(function (context, done) {
boot.ready(function (err, context, done) {
assert.equal(context, server)
done()
})
Expand Down
20 changes: 13 additions & 7 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 @@ -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 @@ -222,14 +226,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
153 changes: 138 additions & 15 deletions test/after-and-ready.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const test = require('tap').test
const boot = require('..')

test('boot a plugin and then execute a call after that', (t) => {
t.plan(4)
t.plan(5)

const app = boot()
let pluginLoaded = false
Expand All @@ -16,7 +16,8 @@ test('boot a plugin and then execute a call after that', (t) => {
done()
})

app.after(function (cb) {
app.after(function (err, cb) {
t.error(err)
t.ok(pluginLoaded, 'afterred!')
afterCalled = true
cb()
Expand All @@ -29,7 +30,7 @@ test('boot a plugin and then execute a call after that', (t) => {
})

test('after without a done callback', (t) => {
t.plan(4)
t.plan(5)

const app = boot()
let pluginLoaded = false
Expand All @@ -41,7 +42,8 @@ test('after without a done callback', (t) => {
done()
})

app.after(function () {
app.after(function (err) {
t.error(err)
t.ok(pluginLoaded, 'afterred!')
afterCalled = true
})
Expand All @@ -53,15 +55,16 @@ test('after without a done callback', (t) => {
})

test('verify when a afterred call happens', (t) => {
t.plan(2)
t.plan(3)

const app = boot()

app.use(function (s, opts, done) {
done()
})

app.after(function (cb) {
app.after(function (err, cb) {
t.error(err)
cb()
}, function () {
t.pass('afterred finished')
Expand All @@ -73,7 +76,7 @@ test('verify when a afterred call happens', (t) => {
})

test('internal after', (t) => {
t.plan(17)
t.plan(18)

const app = boot()
let firstLoaded = false
Expand All @@ -90,7 +93,8 @@ test('internal after', (t) => {
t.notOk(thirdLoaded, 'third is not loaded')
firstLoaded = true
s.use(second)
s.after(function (cb) {
s.after(function (err, cb) {
t.error(err)
t.notOk(afterCalled, 'after was not called')
afterCalled = true
cb()
Expand Down Expand Up @@ -126,14 +130,15 @@ test('internal after', (t) => {
})

test('ready adds at the end of the queue', (t) => {
t.plan(11)
t.plan(14)

const app = boot()
let pluginLoaded = false
let afterCalled = false
let readyCalled = false

app.ready(function (cb) {
app.ready(function (err, cb) {
t.error(err)
t.ok(pluginLoaded, 'after the plugin')
t.ok(afterCalled, 'after after')
readyCalled = true
Expand All @@ -145,15 +150,17 @@ test('ready adds at the end of the queue', (t) => {
t.notOk(readyCalled, 'ready not called')
pluginLoaded = true

app.ready(function () {
app.ready(function (err) {
t.error(err)
t.ok(readyCalled, 'after the first ready')
t.ok(afterCalled, 'after the after callback')
})

done()
})

app.after(function (cb) {
app.after(function (err, cb) {
t.error(err)
t.ok(pluginLoaded, 'executing after!')
t.notOk(readyCalled, 'ready not called')
afterCalled = true
Expand All @@ -168,7 +175,7 @@ test('ready adds at the end of the queue', (t) => {
})

test('if the after/ready callback has two parameters, the first one must be the context', (t) => {
t.plan(2)
t.plan(4)

const server = { my: 'server' }
const app = boot(server)
Expand All @@ -177,13 +184,129 @@ test('if the after/ready callback has two parameters, the first one must be the
done()
})

app.after(function (context, cb) {
app.after(function (err, context, cb) {
t.error(err)
t.equal(server, context)
cb()
})

app.ready(function (context, cb) {
app.ready(function (err, context, cb) {
t.error(err)
t.equal(server, context)
cb()
})
})

test('error should come in the first after - one parameter', (t) => {
t.plan(3)

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

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

app.after(function (err) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
})

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

test('error should come in the first after - two parameters', (t) => {
t.plan(3)

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

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

app.after(function (err, cb) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
cb()
})

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

test('error should come in the first after - three parameter', (t) => {
t.plan(4)

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

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

app.after(function (err, context, cb) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
t.equal(context, server)
cb()
})

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

test('error should come in the first ready - one parameter', (t) => {
t.plan(2)

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

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

app.ready(function (err) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
})
})

test('error should come in the first ready - two parameters', (t) => {
t.plan(2)

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

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

app.ready(function (err, cb) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
cb()
})
})

test('error should come in the first ready - three parameters', (t) => {
t.plan(3)

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

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

app.ready(function (err, context, cb) {
t.ok(err instanceof Error)
t.is(err.message, 'err')
t.equal(context, server)
cb()
})
})
Loading