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.

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
Merged

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

merged 2 commits into from Oct 19, 2016

Conversation

@gordysc
Copy link
Contributor

@gordysc 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
Copy link
Contributor

@galenandrew galenandrew commented Oct 13, 2016

This should resolve #1388

Copy link
Member

@elhigu elhigu left a comment

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
Member

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
Author 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
Member

Also comment here.

@gordysc
Copy link
Contributor Author

@gordysc gordysc commented Oct 19, 2016

@elhigu Comments have been added as per request.

@elhigu elhigu merged commit 718b746 into knex:master Oct 19, 2016
1 check passed
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

* Adding comments for returns
tgriesser added a commit that referenced this pull request Oct 19, 2016
* 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
* 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)
rslabbert added a commit to rslabbert/knex that referenced this pull request Nov 9, 2016
* Adding return's to address warnings mentioned in knex#1388

* Adding comments for returns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants