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

replace Bluebird.timeout #3634

Merged
merged 5 commits into from Feb 12, 2020
Merged

Conversation

maximelkin
Copy link
Collaborator

@maximelkin maximelkin commented Jan 21, 2020

Breaking changes: TImeoutError now not extends Bluebird.TimeoutError
also this error exported explicitly

package.json Outdated
@@ -39,6 +39,7 @@
"liftoff": "3.1.0",
"lodash": "^4.17.15",
"mkdirp": "^0.5.1",
"p-timeout": "^3.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Considering how small p-yimeout is, maybe it would make sense to inline implementation and use native finally instead of p-finally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no strong opinion on that, though, sindresorhus libs are legit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@maximelkin
Copy link
Collaborator Author

Does we still support node 8?

@kibertoad
Copy link
Collaborator

@maximelkin Yeah, ideally we want to support Node 8 for a bit longer. If that makes implementation complex, we can stick with p-timeout, it's not a big deal.

@maximelkin
Copy link
Collaborator Author

This can make harder to move from bluebird completely, because Promise.finally only from 10 node

@kibertoad
Copy link
Collaborator

@maximelkin doesn't p-timeout rely on p-finally which supports node 8?

@maximelkin
Copy link
Collaborator Author

Yes, it is
But p-timeout dependency removed

@kibertoad
Copy link
Collaborator

@maximelkin I'd much rather use p-finally directly or p-timeout rather than keep bluebird or break node 8 compatibility.

@maximelkin
Copy link
Collaborator Author

p-timeout required finally because of support .cancel, we dont need it, so no troubles here

@kibertoad
Copy link
Collaborator

ah, great :)

@kibertoad
Copy link
Collaborator

kibertoad commented Jan 28, 2020

@maximelkin Since #3639 is dropping support for Node 8 anyway, I guess this PR can do the same and we can merge both during next major release.

@maximelkin
Copy link
Collaborator Author

Not directly related question:
bluebird will be replaced completely to es6 promises, or user may choose api/internal promises?

@elhigu
Copy link
Member

elhigu commented Jan 29, 2020

bluebird will be replaced completely to es6 promises, or user may choose api/internal promises?

Knex will use only native promises and will not allow to choose the implementation.

@maximelkin maximelkin mentioned this pull request Feb 12, 2020
@maximelkin
Copy link
Collaborator Author

@kibertoad This pr actually not breaking node 8 compatibility, maybe we can merge it?

@kibertoad kibertoad merged commit 88d832c into knex:master Feb 12, 2020
@kibertoad
Copy link
Collaborator

@maximelkin There's a new unhandled Rejection on Node 8 happening ever since this was merged:

{ reason: Error: Transaction rejected with non-error: undefined
    at q.trxClient.query.catch.then.res (/home/travis/build/knex/knex/lib/transaction.js:15:2936)
From previous event:
    at Transaction._promise.acquireConnection.connection (/home/travis/build/knex/knex/lib/transaction.js:8:1205)
    at Bluebird.then.then (/home/travis/build/knex/knex/lib/transaction.js:18:1002)
From previous event:
    at Transaction.acquireConnection (/home/travis/build/knex/knex/lib/transaction.js:18:886)
    at new Transaction (/home/travis/build/knex/knex/lib/transaction.js:8:834)
    at Client_SQLite3.transaction (/home/travis/build/knex/knex/lib/client.js:7:2337)
    at Function.transaction (/home/travis/build/knex/knex/lib/util/make-knex.js:6:103)
    at Context.it (/home/travis/build/knex/knex/test/unit/knex.js:403:28)
    at callFn (/home/travis/build/knex/knex/node_modules/mocha/lib/runnable.js:395:21)
    at Test.Runnable.run (/home/travis/build/knex/knex/node_modules/mocha/lib/runnable.js:382:7)
    at Runner.runTest (/home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:541:10)
    at /home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:667:12
    at next (/home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:450:14)
    at /home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:460:7
    at next (/home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:362:14)
    at Immediate._onImmediate (/home/travis/build/knex/knex/node_modules/mocha/lib/runner.js:428:5)
    at runCallback (timers.js:810:20)
    at tryOnImmediate (timers.js:768:5)
    at processImmediate [as _immediateCallback] (timers.js:745:5) }

Wonder if there's a gap in our timeout implementation and if sindresorhus one would be more reliable.

@maximelkin
Copy link
Collaborator Author

@kibertoad
I think it somehow connected with refactoring from bluebird pattern-match by class .catch to es6 .catch

@maximelkin maximelkin deleted the remove_bluebird_timeout branch February 14, 2020 09:52
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.

None yet

3 participants