Skip to content

Make .then() resolve to the server instance#114

Merged
mcollina merged 4 commits intomasterfrom
resolve-to-self
Jul 6, 2020
Merged

Make .then() resolve to the server instance#114
mcollina merged 4 commits intomasterfrom
resolve-to-self

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Jul 6, 2020

Fixes fastify/fastify#2374

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

t.pass('plugin2 init')
return Promise.resolve()
}).then(() => {
}).then((f2) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The reachable pass assert is redundant now

@@ -0,0 +1,11 @@
'use strict'

Copy link
Contributor

Choose a reason for hiding this comment

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

File name has a typo

Copy link
Contributor

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

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

Overall change lgtm, just nits

const app = {}
boot(app)

t.is(await app, app)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you call await app multiple times?

Suggested change
t.is(await app, app)
t.is(await app, app)
t.is(await app, app)
t.is(await app, app)

Copy link
Member Author

Choose a reason for hiding this comment

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

It all works fine.

@mcollina
Copy link
Member Author

mcollina commented Jul 6, 2020

@delvedor @davidmarkclements PTAL

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina dismissed davidmarkclements’s stale review July 6, 2020 17:12

feedback addressed

@mcollina mcollina merged commit 060a358 into master Jul 6, 2020
@mcollina mcollina deleted the resolve-to-self branch July 6, 2020 17:58
@rqbazan
Copy link

rqbazan commented Jul 6, 2020

Amazing support! 👏

@mcollina
Copy link
Member Author

mcollina commented Jul 6, 2020

thanks! It's not normally this fast but this was a really bad one that I forgot to fix.

Copy link
Contributor

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

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

Lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initialization error in v3.0.0-rc.5

5 participants