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

Adding return's to address warnings mentioned in #1388 #1740

Merged
merged 2 commits into from Oct 19, 2016

Conversation

Projects
None yet
3 participants
@gordysc
Contributor

gordysc commented Oct 13, 2016

This addresses warnings generated because we don't explicitly return a promise as mentioned in Bluebird's API documentation:
http://bluebirdjs.com/docs/warning-explanations.html#warning-a-promise-was-created-in-a-handler-but-was-not-returned-from-it

Stack trace is:

(node:15) Warning: a promise was created in a handler at /app/node_modules/hapi/lib/auth.js:319:49 but was not returned from it, see http://goo.gl/rRqMUw
    at Function.Promise.attempt.Promise.try (/app/node_modules/bluebird/js/release/method.js:29:9)
    at process.nextTick (/app/node_modules/hoek/lib/index.js:854:22)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickDomainCallback (internal/process/next_tick.js:122:9)
From previous event:
    at Runner.ensureConnection (/app/node_modules/knex/lib/runner.js:186:39)
    at Runner.run (/app/node_modules/knex/lib/runner.js:41:44)
    at QueryBuilder.Target.then (/app/node_modules/knex/lib/interface.js:32:43)
    at QueryBuilder.Target.(anonymous function) [as map] (/app/node_modules/knex/lib/interface.js:83:23)
@galenandrew

This comment has been minimized.

Contributor

galenandrew commented Oct 13, 2016

This should resolve #1388

@elhigu

requires couple of comments to prevent this from breaking again.

@@ -183,7 +183,7 @@ assign(Runner.prototype, {
ensureConnection() {
return Promise.try(() => {
return this.connection || new Promise((resolver, rejecter) => {
this.client.acquireConnection()
return this.client.acquireConnection()

This comment has been minimized.

@elhigu

elhigu Oct 19, 2016

Collaborator

This place needs comment to code, which says something like "need to return promise or null from handler to prevent warning from bluebird"

Otherwise this return statement might be removed by accidents, since it doesn't serve any other purpose here.

This comment has been minimized.

@gordysc

gordysc Oct 19, 2016

Contributor

Okay...this seems like a best practice though and we aren't explicitly calling it out elsewhere? I can certainly add the comments

@@ -195,7 +195,7 @@ assign(Runner.prototype, {
.catch(rejecter)
})
}).disposer(() => {
this.client.releaseConnection(this.connection)
return this.client.releaseConnection(this.connection)

This comment has been minimized.

@elhigu

elhigu Oct 19, 2016

Collaborator

Also comment here.

@gordysc

This comment has been minimized.

Contributor

gordysc commented Oct 19, 2016

@elhigu Comments have been added as per request.

@elhigu elhigu merged commit 718b746 into tgriesser:master Oct 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

tgriesser added a commit that referenced this pull request Oct 19, 2016

Adding return's to address warnings mentioned in #1388 (#1740)
* Adding return's to address warnings mentioned in #1388

* Adding comments for returns

tgriesser added a commit that referenced this pull request Oct 19, 2016

Merge branch '0.12.x'
* 0.12.x:
  release 0.12.6
  Update changelog
  Remove postinstall script for now
  Gitignore yarn.lock
  Adding return's to address warnings mentioned in #1388 (#1740)

tgriesser added a commit that referenced this pull request Oct 19, 2016

Merge branch 'master' into refactor
* master:
  release 0.12.6
  Update changelog
  Remove postinstall script for now
  Gitignore yarn.lock
  Adding return's to address warnings mentioned in #1388 (#1740)
  Adding return's to address warnings mentioned in #1388 (#1740)

RubenSlabbert added a commit to RubenSlabbert/knex that referenced this pull request Nov 9, 2016

Adding return's to address warnings mentioned in tgriesser#1388 (tgri…
…esser#1740)

* Adding return's to address warnings mentioned in tgriesser#1388

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