Add support for `ready` to return a promise. #37

coreyfarrell · 2017-12-06T21:44:21Z

No description provided.

mcollina · 2017-12-06T22:05:10Z

README.md

@@ -234,9 +234,11 @@ The callback changes basing on the parameters your are giving:
 3. If two parameters are given to the callback, the first will be the `error` object, the second will be the `done` callback.
 4. If three parameters are given to the callback, the first will be the `error` object, the second will be the top level `context` unless you have specified both server and override, in that case the `context` will be what the override returns, and the third the `done` callback.

+If no callback is provided `ready` will return a Promise that is resolved or rejected once plugins and `after` calls are completed.  No data is provided to the `.then` callback, if an error occurs it is provided to the `.catch` callback.


I think we should provide the context to resolve.

I've updated my working copy code to do this. I'll update the docs and push the update once it's decided what should be returned from app.ready(callbackForm).

This has not been updated

allevo · 2017-12-06T22:05:39Z

Sorry for the intrusion

I'm 👍 for moving ready for a better async support

I'm 👎 for return different kind of type depending on a parameter presence

mcollina · 2017-12-06T22:07:25Z

@allevo you propose to return null if we are not generating a Promise? That'll be a semver-major change.

allevo · 2017-12-06T22:16:48Z

That'll be a semver-major change.

Yeah. And it could be a problem. But returning different type is very a bad thing. null could be a solution.

For example the mongodb library return a Promise or undefined (no return statement). See here for example

mcollina · 2017-12-06T22:34:18Z

If we do a semver-major, we might want to do some more work here to better support async/await overall.

delvedor

I'm ok with this change, but we should add a check to verify that a user does not register other plugins after the ready execution.

coreyfarrell · 2017-12-16T02:54:55Z

Should I make ready return null or undefined when not returning a Promise?

@delvedor So I make sure I understand correctly you want me to add a check to Boot.prototype.use:

if (this.booted) {
  throw new Error('plugin cannot be loaded after ready')
}

Should that be done in a separate commit so it appears in git log --oneline?

mcollina · 2017-12-16T10:22:25Z

Should I make ready return null or undefined when not returning a Promise?

Yes, I'd prefer undefined.

@coreyfarrell the check that @delvedor wants is already part of https://github.com/mcollina/avvio/pull/38/files#diff-fe60a8aed4b3103661f9d88361346b0dR185. They will be released together anyway. Go ahead and just do the change on ready().

coreyfarrell · 2017-12-16T19:34:48Z

Returning undefined helped me catch the fact that I missed the wrap() function while writing this patch. I've fixed that for ready but it also shows that tests/async-await.js fails to test the wrap feature using:

const app = {}
boot(app)

Right now I'm manually switching the tests in so I can find/fix the issues with wrap async. I haven't looked into what it would take to have async-await.js run each test twice (once with and once without wrap).

coreyfarrell · 2017-12-17T23:23:11Z

I'm now realizing github probably didn't send any notifications when I updated the PR yesterday. I've addressed feedback and fixed issues found by testing with wrapped objects.

mcollina · 2017-12-18T07:43:27Z

README.md

@@ -234,9 +234,11 @@ The callback changes basing on the parameters your are giving:
 3. If two parameters are given to the callback, the first will be the `error` object, the second will be the `done` callback.
 4. If three parameters are given to the callback, the first will be the `error` object, the second will be the top level `context` unless you have specified both server and override, in that case the `context` will be what the override returns, and the third the `done` callback.

+If no callback is provided `ready` will return a Promise that is resolved or rejected once plugins and `after` calls are completed.  No data is provided to the `.then` callback, if an error occurs it is provided to the `.catch` callback.


This has not been updated

mcollina · 2017-12-18T07:44:44Z

boot.js

@@ -229,8 +228,23 @@ Boot.prototype.close = function (func) {
 }

 Boot.prototype.ready = function (func) {
-  this._readyQ.push(func)
-  return this
+  if (func) {


I would check if this is a function and throw if it exists but it is not.

mcollina · 2017-12-18T07:50:12Z

test/chainable.test.js

@@ -16,10 +16,8 @@ test('chainable standalone', (t) => {
      done()
    }).ready(function () {
      t.pass('we are ready')
-    }).use(function (ctx, opts, done) {
-      t.pass('3rd plugin')
-      done()


Can you restore this?

ready no longer returns this so the test now verifies that it is not chainable.

If you are concerned about testing that use after ready does the right thing I believe other tests do this, ready adds at the end of the queue from after-and-ready.test.js for example calls ready before calling use.

Please let me know if I'm misunderstanding your finding.

mcollina · 2017-12-18T07:50:21Z

test/chainable.test.js

@@ -38,10 +36,8 @@ test('chainable automatically binded', (t) => {
      done()
    }).ready(function () {
      t.pass('we are ready')
-    }).use(function (ctx, opts, done) {
-      t.pass('3rd plugin')
-      done()


can you restore this?

coreyfarrell mentioned this pull request Dec 6, 2017

Better async/await support fastify/fastify#481

Closed

mcollina reviewed Dec 6, 2017

View reviewed changes

delvedor reviewed Dec 11, 2017

View reviewed changes

allevo mentioned this pull request Dec 15, 2017

v1.0.0 needs to be issued fastify/fastify#229

Closed

19 tasks

mcollina reviewed Dec 18, 2017

View reviewed changes

coreyfarrell and others added 2 commits December 18, 2017 14:43

Add support for ready to return a promise.

a70fbcd

Merge branch 'master' into await-ready

a1668fc

mcollina merged commit a1668fc into fastify:master Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `ready` to return a promise. #37

Add support for `ready` to return a promise. #37

coreyfarrell commented Dec 6, 2017

mcollina Dec 6, 2017

coreyfarrell Dec 6, 2017

mcollina Dec 18, 2017

allevo commented Dec 6, 2017

mcollina commented Dec 6, 2017 •

edited

Loading

allevo commented Dec 6, 2017

mcollina commented Dec 6, 2017 •

edited

Loading

delvedor left a comment

coreyfarrell commented Dec 16, 2017

mcollina commented Dec 16, 2017

coreyfarrell commented Dec 16, 2017

coreyfarrell commented Dec 17, 2017

mcollina Dec 18, 2017

mcollina Dec 18, 2017

mcollina Dec 18, 2017

coreyfarrell Dec 18, 2017

mcollina Dec 18, 2017

Add support for ready to return a promise. #37

Add support for ready to return a promise. #37

Conversation

coreyfarrell commented Dec 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allevo commented Dec 6, 2017

mcollina commented Dec 6, 2017 • edited Loading

allevo commented Dec 6, 2017

mcollina commented Dec 6, 2017 • edited Loading

delvedor left a comment

Choose a reason for hiding this comment

coreyfarrell commented Dec 16, 2017

mcollina commented Dec 16, 2017

coreyfarrell commented Dec 16, 2017

coreyfarrell commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for `ready` to return a promise. #37

Add support for `ready` to return a promise. #37

mcollina commented Dec 6, 2017 •

edited

Loading

mcollina commented Dec 6, 2017 •

edited

Loading