NODE-2579/3.6/global-promise-prereq #2340

reggi · 2020-05-01T14:50:34Z

Implements promise-provider and gets rid of native calls to Promise, each call needs to be defined within scope uses eslint-plugin-promise to enforce.

Because this is a large commit I tried to have a simple commits to make it easier to review:

adds promise_provider

This commit adds the new file promise_provider.

tests use promise-provider

This commit adds require calling promise_provider to the top of every test that uses a Promise, getting rid of global Promise call.

uses promise provider & no-native

This commit makes calls to PromiseProvider.get() in functions. Fixes some missed returns where natives shouldn't have been used. The functionality of PromiseProvider.get is customized to take any number of arguments, these arguments are possible stores for promiseLibrary. Some changes may have been made that changes current functionality to methods, either by adding optional promiseLibrary where it should have existed or by adding it where it shouldn't be, open to changing these cases.

implements `eslint-plugin-promise`

The last commit, adds the eslint-plugin-promise rule, which prevents any use of a Promise that is not defined in scope. Tested and pass lint.

reggi · 2020-05-01T14:52:20Z

lib/collection.js

@@ -128,7 +129,7 @@ function Collection(db, topology, dbName, name, pkFactory, options) {
  const namespace = new MongoDBNamespace(dbName, name);

  // Get the promiseLibrary
-  const promiseLibrary = options.promiseLibrary || Promise;
+  const promiseLibrary = PromiseProvider.get(options, db, topology);


Here's a case that I'm taking about. It's a little more comprehensive than it previously was. I read this as, check the options, then check db, then check topology.

reggi · 2020-05-01T14:53:17Z

lib/cursor.js

-      return new this.s.promiseLibrary(resolve => {
-        resolve();
-      });
+      return Promise.resolve();


This was an opinionated change, no reason to call new here just to resolve.

reggi · 2020-05-01T14:53:51Z

lib/utils.js

@@ -705,11 +707,11 @@ function* makeCounter(seed) {
 * @returns {Promise|void} Returns nothing if a callback is supplied, else returns a Promise.
 */
 function maybePromise(parent, callback, fn) {
-  const PromiseLibrary = (parent && parent.s && parent.s.promiseLibrary) || Promise;
+  const Promise = PromiseProvider.get(parent);


Much cleaner here 😍

reggi · 2020-05-01T14:54:50Z

test/functional/promise_provider.test.js

+  withS: { s: { promiseLibrary: Symbol('withS') } }
+};
+
+describe('PromiseProvider', () => {


This is a test for PromiseProvider just testing for the reducer, ensuring it works as expected.

reggi · 2020-05-01T15:10:03Z

lib/collection.js

@@ -443,7 +444,7 @@ Collection.prototype.find = deprecateOptions(
    newOptions.db = this.s.db;

    // Add the promise library
-    newOptions.promiseLibrary = this.s.promiseLibrary;
+    newOptions.promiseLibrary = PromiseProvider.get(options, this);


This is within collection.find it's not clear to me which methods you can pass in promiseLibrary and which you can't. Should this be PromiseProvider.get(this); or PromiseProvider.get(options, this);? Thoughts?

If I'm understanding things correctly, in order to keep the behavior unchanged here for collection.find, it should be PromiseProvider.get(this); - the original code always sets newOptions.promiseLibrary = this.s.promiseLibrary regardless of the user passing in options.promiseLibrary.

Any methods in Collection which don't call something like options.promiseLibrary = this.s.promiseLibrary; before executing the operation would need to honor the promiseLibrary passed in by the user on that method - if there are any such methods.

reggi · 2020-05-01T17:09:09Z

lib/admin.js

@@ -42,14 +42,14 @@ const executeOperation = require('./operations/execute_operation');
 * @class
 * @return {Admin} a collection instance.
 */
-function Admin(db, topology, promiseLibrary) {


Had the idea of removing this promiseLibrary option, and rather just pull it out of db.

mbroadst · 2020-05-02T11:42:20Z

lib/admin.js

  if (!(this instanceof Admin)) return new Admin(db, topology);

  // Internal state
  this.s = {
    db: db,
    topology: topology,
-    promiseLibrary: promiseLibrary
+    promiseLibrary: PromiseProvider.get(db, topology)


I thought the idea was to remove every instance of promiseLibrary in favor of getting it from the PromiseProvider?

It is my understanding that each time a method takes-in promiseLibrary it set's it for it's class-scope and it's-children. And I've tried to illustrate this in this example. I thought we were all in-agreement that 3.6 can't change this functionality.

// here mongoClient sets `bluebird` const client = await MongoClient("mongodb://127.0.0.1", { promiseLibrary: bluebird, useUnifiedTopology: true }); const db = client.db("test"); // here in collection I'm setting the promiseLibrary to `q` const collectionPromise = db.collection("test", { promiseLibrary: q }).find().toArray();

I would assume here the driver would provide any promises coming from client to be bluebird and promises coming from collectionPromise to be q.

This is the "parody" i believed we we were trying to support in 3.6

If we use a "global" setter / getter, and don't store in-parent-class and pass options, we loose this tracking of what class is using what.

My plan was to remove the stores in master and only .set() in MongoClient (and mognodb.Promise = bluebird module-getter)

This is related to the private-slack thread here.

mbroadst · 2020-05-02T11:46:25Z

lib/collection.js

  if (typeof options === 'function') (callback = options), (options = {});
  options = options || {};

  const err = checkForAtomicOperators(update);
  if (err) {
    if (typeof callback === 'function') return callback(err);
-    return this.s.promiseLibrary.reject(err);
+    return Promise.reject(err);


these are frustrating.. they should really be deferred to the execute method of UpdateOneOperation

Should that change be in this PR?

no, just calling it out

mbroadst · 2020-05-02T11:47:56Z

lib/core/sdam/topology.js

@@ -193,7 +195,7 @@ class Topology extends EventEmitter {
      // Active client sessions
      sessions: new Set(),
      // Promise library
-      promiseLibrary: options.promiseLibrary || Promise,
+      promiseLibrary: PromiseProvider.get(options),


A user can't create a Topology, so the promiseLibrary provided to this class is only whatever is set on the top level MongoClient. I think we can remove storage here, and just depend on the PromiseProvider.get

mbroadst · 2020-05-02T11:50:52Z

lib/gridfs-stream/index.js

@@ -51,7 +52,7 @@ function GridFSBucket(db, options) {
    _filesCollection: db.collection(options.bucketName + '.files'),
    checkedIndexes: false,
    calledOpenUploadStream: false,
-    promiseLibrary: db.s.promiseLibrary || Promise
+    promiseLibrary: PromiseProvider.get(db)


I don't see any need to store the promiseLibrary for cases where it's not already being accepted in options. In this case the db is being stored so I think you can replace future uses in this class with PromiseProvider.get(this.s.db)

mbroadst · 2020-05-02T11:53:19Z

lib/promise_provider.js

+ */
+
+const store = {
+  _promise: null


please use a Symbol instead of a underscore prefixed value, we don't want people changing this by hand:

const kPromise = Symbol('promise'); const Store { [kPromise]: undefined }

mbroadst · 2020-05-02T11:57:34Z

lib/promise_provider.js

+ * @api private
+ */
+
+const store = {


any reason you used an object instance here, rather than a class with static methods?

class PromiseProvider { static get(...) {} } const kPromise = Symbol('promise'); PromiseProvider[kPromise] = undefined;

I ended up copping mongooses implementation directly after previous comments regarding preferring their file-name convention, and scrapping the getter implementation I had earlier developer. I thought we were trying not to veer off what they had too much. I'd be down to use a class.

let's use a class!

mbroadst · 2020-05-02T11:58:50Z

test/functional/promise_provider.test.js

@@ -0,0 +1,41 @@
+'use strict';


nit: I think this is a unit test, not a functional one

mbroadst · 2020-05-02T12:01:58Z

test/benchmarks/driverBench/index.js

@@ -1,5 +1,6 @@
 'use strict';

+const Promise = require('../../../lib/promise_provider').get();


is it possible to use ignorePatterns to ignore files in test for this rule? I don't think it buys us anything to force these declarations in our test files, we only want it in our lib files to provide support to our users.

This is the opposite of what you mentioned earlier regarding adding these to tests because it' allows us to test the driver using multiple promise implementations.

I'm not sure if it's possible for eslint to ignore a specific rule (eslint-plugin-promise) for a specific folder (tests). Without possibly having two different eslint configs.

If there's a way to not have to import this in tests, totally I'm down.

emadum

Whew, this is a big one! I didn't get all the way through the PR, but I'll stop here pending feedback on the items I raised.

emadum · 2020-05-02T15:12:32Z

lib/promise_provider.js

+
+store.get = function() {
+  const values = Object.keys(arguments).map(k => arguments[k]);
+  const result = values.reduce((acq, parent) => {


This is probably negligible in terms of performance, but I'd prefer doing this without the extra array traversal. E.g.

const result = Object.keys(arguments).reduce((acq, k) => { const parent = arguments[k]; ...

Thanks, adopting change.

emadum · 2020-05-02T15:41:30Z

lib/change_stream.js

@@ -1,5 +1,6 @@
 'use strict';

+const PromiseProvider = require('./promise_provider');


Just wanted to point out that this class has been refactored to use maybePromise so most of these changes will cause merge conflicts once #2333 makes its way into 3.6. It might be easier to just revert this file and add a /* es-lint-disable promise/no-native */ at the top, along with a note to update once that PR is merged in - not a big deal either way.

emadum · 2020-05-02T15:54:56Z

lib/collection.js

@@ -443,7 +444,7 @@ Collection.prototype.find = deprecateOptions(
    newOptions.db = this.s.db;

    // Add the promise library
-    newOptions.promiseLibrary = this.s.promiseLibrary;
+    newOptions.promiseLibrary = PromiseProvider.get(options, this);


If I'm understanding things correctly, in order to keep the behavior unchanged here for collection.find, it should be PromiseProvider.get(this); - the original code always sets newOptions.promiseLibrary = this.s.promiseLibrary regardless of the user passing in options.promiseLibrary.

Any methods in Collection which don't call something like options.promiseLibrary = this.s.promiseLibrary; before executing the operation would need to honor the promiseLibrary passed in by the user on that method - if there are any such methods.

emadum · 2020-05-02T16:07:17Z

lib/collection.js

@@ -1990,7 +1994,7 @@ Collection.prototype.parallelCollectionScan = deprecate(function(options, callba
  options.readPreference = resolveReadPreference(this, options);

  // Add a promiseLibrary
-  options.promiseLibrary = this.s.promiseLibrary;


I'm a little confused about what setting options.promiseLibrary before running executeLegacyOperation does. As you can see here, executeLegacyOperation ignores the options and grabs the promiseLibrary from the topology. If I'm not missing something, we can just delete anything that sets options.promiseLibrary before a call to executeLegacyOperation.

Thomas Reggi added 4 commits May 1, 2020 10:30

adds promise-provider

158c141

tests use promise-provider

32eb671

uses promise provider & no-native

9960092

implements eslint-plugin-promise

d3e8428

reggi commented May 1, 2020

View reviewed changes

reggi requested review from mbroadst and emadum May 1, 2020 14:55

reggi commented May 1, 2020

View reviewed changes

rm Admin promiseLibrary property

a6bc216

reggi commented May 1, 2020

View reviewed changes

mbroadst suggested changes May 2, 2020

View reviewed changes

emadum suggested changes May 2, 2020

View reviewed changes

use class, symbol, validator, fix test

75278d0

reggi closed this May 4, 2020

reggi deleted the NODE-2579/3.6/global-promise-prereq branch June 2, 2020 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NODE-2579/3.6/global-promise-prereq #2340

NODE-2579/3.6/global-promise-prereq #2340

reggi commented May 1, 2020

reggi May 1, 2020 •

edited

Loading

reggi May 1, 2020

reggi May 1, 2020

reggi May 1, 2020

reggi May 1, 2020

emadum May 2, 2020

reggi May 1, 2020 •

edited

Loading

mbroadst May 2, 2020

reggi May 4, 2020 •

edited

Loading

mbroadst May 2, 2020

reggi May 4, 2020

mbroadst May 4, 2020

mbroadst May 2, 2020

mbroadst May 2, 2020

mbroadst May 2, 2020

mbroadst May 2, 2020

reggi May 4, 2020

mbroadst May 4, 2020

mbroadst May 2, 2020

mbroadst May 2, 2020

reggi May 4, 2020 •

edited

Loading

emadum left a comment

emadum May 2, 2020

reggi May 4, 2020

emadum May 2, 2020

emadum May 2, 2020

emadum May 2, 2020

		@@ -1,5 +1,6 @@
		'use strict';

		const Promise = require('../../../lib/promise_provider').get();

		@@ -1,5 +1,6 @@
		'use strict';

		const PromiseProvider = require('./promise_provider');

NODE-2579/3.6/global-promise-prereq #2340

NODE-2579/3.6/global-promise-prereq #2340

Conversation

reggi commented May 1, 2020

adds promise_provider

tests use promise-provider

uses promise provider & no-native

implements eslint-plugin-promise

reggi May 1, 2020 • edited Loading

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

reggi May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reggi May 4, 2020 • edited Loading

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

reggi May 4, 2020 • edited Loading

Choose a reason for hiding this comment

emadum left a 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

implements `eslint-plugin-promise`

reggi May 1, 2020 •

edited

Loading

reggi May 1, 2020 •

edited

Loading

reggi May 4, 2020 •

edited

Loading

reggi May 4, 2020 •

edited

Loading