remove bluebird #3650

maximelkin · 2020-02-05T11:06:17Z

Closing #1588

maximelkin · 2020-02-12T09:39:14Z

Wait for #3634

maximelkin · 2020-02-13T23:50:03Z

todo one of:

moving tests from bluebird
global or knex instance promise lib setup
2.1) adding methods in runtime vs adding another level of prototype at knex instantiation
2.2) searching for all external async api methods and wrapping with user defined lib

qubyte · 2020-02-14T08:57:59Z

lib/client.js

-        });
-    } catch (e) {
-      return Bluebird.reject(e);
+      return Promise.reject(new Error('Unable to acquire a connection'));


No need for an explicit Promise.reject here. Since this is an async function, all you have to do is throw the error and the caller will see the promise this function returns become rejected.

Similar for the promise chain below. Better to await etc.

briandamaged · 2020-02-21T00:27:05Z

Howdy! Just curious: what's the intended scope of this PR?

Remove the dependency on the Bluebird library?
Rewrite existing logic to take advantage of async / await?
Something else?

The reason I'm asking: this seems like it could potentially touch a lot of code, which will make it challenging to code review. So, perhaps the overall effort can be split into several smaller PRs?

kibertoad · 2020-02-21T00:33:43Z

I agree with the idea of smaller PRs being better and this is something we should in general strive for; however, at this point it's likely that PR split is going to take non-trivial effort; if that is the case, I'd rather review a larger PR than lose part of refactoring due to simple revert.

kibertoad · 2020-02-21T00:41:36Z

That said, since PR is pretty large already, it might make sense to wrap up what was already changed into final state, review, merge and continue in a separate PR, rather than keep expanding this one.

maximelkin · 2020-02-21T01:00:02Z

In some places conversion from promise chain to async/await done because of node 8 support and .finally usage
If we can remove node 8 support, some changes can be reverted to make pr smaller

lib/interface.js

briandamaged · 2020-02-21T16:32:07Z

In terms of the "smaller PR" discussion: we don't necessarily need to revert any changes. Instead, we could use this branch as a basis for generating smaller incremental patches:

https://stackoverflow.com/questions/28192623/create-patch-or-diff-file-from-git-repository-and-apply-it-to-another-different

For example:

Generate a patch that includes only "trivial changes" within the /lib folder. (Ex: files where you only had to rename Bluebird.resolve to Promise.resolve, etc. Also: notice that the /test folder is remaining untouched. This will give us confidence that the new code is still working correctly)
Generate a patch that includes some of the "nontrivial changes" within the /lib folder. (Ex: places where code had to be rewritten because it relied upon a Bluebird feature. Once again: the /test folder remains untouched). You might need to repeat this step depending upon the complexity of the changes.
Generate a patch for the /lib folder.

There might be more steps as well, of course. The main concept: you don't need to manually recreate the changes. Instead, you can use this branch as the basis for generating multiple patch sets. This will then make it significantly easier to review and merge the code.

I can assist you with this if you'd like to give it a try.

briandamaged · 2020-02-21T17:26:37Z

Side note: I also see why this is such a challenging PR. I just replaced a Bluebird Promise in 1 line of code, and suddenly 15 seemingly-unrelated sections of the code exploded. 🙀

It turns out that those sections of the code all relied upon the .finally(..) method at some point within their control flow. So, I temporarily re-wrapped the Promise w/ Bluebird.resolve(..) just to work around the issue.

kibertoad · 2020-02-21T19:49:09Z

test/utils.js

@@ -0,0 +1,8 @@
+const assert = require('assert');


could be chai for consistency

code removed

kibertoad · 2020-02-21T19:50:35Z

test/tape/invalid-db-setup.js

              t.ok(
-                e.message.indexOf('i-refuse-to-exist') > 0,
+                e.message.includes('i-refuse-to-exist'),


kibertoad · 2020-02-21T19:55:39Z

Preferably we keep Node 8 support until Node 14 lands.

briandamaged · 2020-02-21T19:59:08Z

test/utils.js

+const assert = require('assert');
+
+const expectError = (promise) =>
+  promise.then(() => assert.fail("Shouldn't have gotten here."), (e) => e);


You might also consider using the chai-as-promised plugin:

https://www.chaijs.com/plugins/chai-as-promised/

This encapsulates the pattern of "failing when an exception didn't occur".

briandamaged · 2020-02-21T20:11:08Z

lib/interface.js

-      };
-    }
-  );
+  availablePromiseMethods().forEach(function(method) {


The list produced by availablePromiseMethods() is significantly shorter than the list that it's replacing. Was this intentional? (Granted: I'm assuming that many of the methods in the original list were Bluebird-specific)

Yes, this methods are from bluebird

briandamaged · 2020-02-21T20:17:32Z

lib/transaction.js

+
+  then(onResolve, onReject) {
+    return this._promise.then(onResolve, onReject);
+  }


FYI: this is probably related to the "availablePromiseMethods() list is significantly shorter than the list that it's replacing" concern that I mentioned elsewhere. The availablePromiseMethods() method is currently omitting "then".

In other words: if the list produced by availablePromiseMethods() included "then", then you probably could just rely upon meta-programming to generate this method automatically.

(On the flipside: given that the list of Promise methods only contains 2-3 elements now, it might no longer be worth leveraging meta-programming for this.)

This will not work, because interface has non-trivial then (but this can be replaced to some internal method too, but this too complex

helper which generates array of promise methods helps with optional .finally

Right -- we're saying the same thing. Basically: you can't use meta-programming for this specific case. However, given that there are only a handful of cases now, it might be worth discarding the meta-programming in favor of something more explicit.

done: 9ef6369
Sorry for delay, didn't quickly realize your proposed solution

briandamaged · 2020-02-21T20:23:24Z

lib/util/promises.js

@@ -0,0 +1,5 @@
+function availablePromiseMethods() {
+  return Promise.prototype.finally ? ['catch', 'finally'] : ['catch'];


Hmm... based upon the Knex documentation, it looks like the .finally(..) method is intentionally being made available to older versions of Node. (Specifically: release notes for version 0.20.2). So, perhaps we should eliminate the conditional logic here and just always provide the .finally(..) method? (Of course, the meta-programming might interfere with this. So... perhaps we should avoid meta-programming for generating these methods?)

0.20.2 is about typings only, as I can see

These release notes for 0.20.2 on http://knexjs.org/ mention this:

Fix regression in older version of node when Promise#finally was not available #3507

(Perhaps this line was omitted from another copy of the release notes?)

I checked #3507 pr

maximelkin · 2020-02-22T00:27:08Z

I can't change lib/ code without changing test/, because tests depends on .finally, which is incompatible with node 8

maximelkin · 2020-02-22T00:28:44Z

lib/dialects/oracle/index.js

      this.driver.connect(this.connectionSettings, (err, connection) => {
        if (err) return rejecter(err);
-        Bluebird.promisifyAll(connection);
+        connection.executeAsync = promisify(connection.execute);


here attention needed: check all file and notice, what from connection only execute (with suffix async) is used

FYI: I discovered earlier today that lib/dialects/oracle is actually partially-dead code. It is not used anywhere except as a parent class for lib/dialects/oracledb.

So, you might want to double-check that this method is even used by lib/dialects/oracledb. If not, then it might be easiest to just remove the entire method.

It's easier to drop support for https://www.npmjs.com/package/oracle entirely, but this is not logical part of this merge request

maximelkin · 2020-02-22T00:32:34Z

lib/dialects/oracledb/index.js

-        function(value, index) {
-          const OutBindsOffset = index * modifiedRowsCount;
-          updatedOutBinds.push(outBinds[i + OutBindsOffset]);
+  return connection


github is bad at diffing with spaces: here only Bluebird.resolve removed

maximelkin · 2020-02-22T00:37:34Z

test/integration/builder/inserts.js

@@ -1148,39 +1147,37 @@ module.exports = function(knex) {
        if (/redshift/i.test(knex.client.driverName)) {
          return;
        }
+        this.timeout(10000);


previously this was timeout on bluebird chain

briandamaged · 2020-02-22T17:56:45Z

@maximelkin + @kibertoad : I've been reviewing the code, but the huge number of changes makes it difficult to be very confident about the code review (fun fact: it's actually 63 pages worth of changes). Would it be possible to split this into at least 2 separate PRs? In particular:

The first PR contains only the revisions to the /test code. This way, we can at least be confident that the old code works with the new test cases. (In other words: this helps us reduce the possibility of having 2 bugs that cancel one another out. Ex: a bug in the /src code that gets ignored due to a bug in the /test code)
The second PR contains the revisions to the actual code. (And ideally: no more changes to the test cases. This way, we can be confident that the code is doing the same thing before and after the changes)

(Ideally, I'd really like to see each of these split into 2-3 separate PRs as well. But, splitting things this way will at least allow us to be a lot more confident in the code reviews).

In terms of strategy, you can do the following:

Create a copy of your branch, and rebase it against master:

git checkout bluebird_removal
git checkout -b bluebird_removal-rebase
git rebase -p master

(This will replay your commits against master, which ensures that we'll get a clean diff in the next step)

Create a patch of the changes against master:

git diff master > bluebird_removal.patch

Switch back to master, and apply only the test-related changes from the patch:

git checkout master
git apply --include="test/*" bluebird_removal.patch

Reintroduce the chai-as-promised dependency to package.json. (Note: we can't use the branch's package.json since it also removed bluebird)

npm install --save-dev chai-as-promised

Create a new branch for these changes:

git checkout -b bluebird_removal-tests_only

At this point, it should be possible to run the tests against the older code. Once we're confident that the tests are good, we can merge them. Afterwards, we can apply the patch to create a new PR that contains the other half of the changes:

# Note: We're assuming that the test changes have already been merged into master
git checkout master
git apply --exclude="test/*" --exclude="package.json" bluebird_removal.patch

Again: I apologize for asking you to do this, but it would really allow everybody to be a lot more confident about the changes that were implemented.

maximelkin · 2020-02-23T11:13:06Z

Split on 2 prs
#3683
#3682

maximelkin force-pushed the bluebird_removal branch from d648c2c to 142ab3a Compare February 13, 2020 23:24

qubyte reviewed Feb 14, 2020

View reviewed changes

maximelkin force-pushed the bluebird_removal branch from 1ec999f to 6f49b74 Compare February 20, 2020 19:37

maximelkin added 13 commits February 21, 2020 04:09

remove bluebird: initial

8dd0cb3

remove bluebird: in src

e3616fc

node 8 compatibility

1bcc21f

fix call

98cd175

asyncify acquireConnection

6d47973

asyncify more things

f2c89a7

remove timeout

3a3c23d

fix asCallback

3431206

bluebird free tests

c26af7c

remove bluebird from build

37c1109

remove bluebird from stress test

2eb2490

remove bluebird from deps

932daf2

fix crossdb-compatibility test

0a05034

maximelkin force-pushed the bluebird_removal branch from ba17813 to 0a05034 Compare February 21, 2020 01:10

maximelkin commented Feb 21, 2020

View reviewed changes

lib/interface.js Outdated Show resolved Hide resolved

maximelkin and others added 4 commits February 21, 2020 11:18

fix asCallback

507bfc2

fix isPending

82182ef

fix invalid db setup test

feab665

fix plan for propagate error

c147f40

maximelkin marked this pull request as ready for review February 21, 2020 11:24

maximelkin changed the title ~~WIP: remove bluebird~~ remove bluebird Feb 21, 2020

kibertoad reviewed Feb 21, 2020

View reviewed changes

briandamaged reviewed Feb 21, 2020

View reviewed changes

maximelkin added 2 commits February 22, 2020 03:16

remove expectError

297ed15

typings: remove bluebird methods

9f76215

maximelkin commented Feb 22, 2020

View reviewed changes

fix linter

8b33ec8

This was referenced Feb 23, 2020

remove dependency on bluebird methods from tests #3682

Merged

remove dependency on bluebird methods from sources #3683

Merged

maximelkin closed this Feb 23, 2020

		@@ -0,0 +1,5 @@
		function availablePromiseMethods() {
		return Promise.prototype.finally ? ['catch', 'finally'] : ['catch'];

remove bluebird #3650

remove bluebird #3650

Conversation

maximelkin commented Feb 5, 2020 • edited

maximelkin commented Feb 12, 2020

maximelkin commented Feb 13, 2020

qubyte Feb 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandamaged commented Feb 21, 2020

kibertoad commented Feb 21, 2020

kibertoad commented Feb 21, 2020

maximelkin commented Feb 21, 2020 • edited

briandamaged commented Feb 21, 2020

briandamaged commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibertoad commented Feb 21, 2020

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximelkin commented Feb 22, 2020

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

briandamaged commented Feb 22, 2020

maximelkin commented Feb 23, 2020 • edited

maximelkin commented Feb 5, 2020 •

edited

qubyte Feb 14, 2020 •

edited

maximelkin commented Feb 21, 2020 •

edited

maximelkin commented Feb 23, 2020 •

edited