refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406

nbbeeken · 2022-09-12T20:19:00Z

Description

What is changing?

Adds maybeCallback helper with tests
Changes PromiseProvider to communicate an unset state
Refactors:
- ChangeStream
- ClientSession
- GridFS

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

baileympearson

General comment - I think we should use maybeCallback in cases we don't use maybePromise now, but use the promise provider. We should encapsulate all knowledge of the promise provider into maybeCallback.

test/integration/node-specific/custom_promise_library.test.js

test/integration/node-specific/cursor_async_iterator.test.js

baileympearson · 2022-09-13T17:36:23Z

test/types/community/client.test-d.ts

@@ -35,7 +35,6 @@ const options: MongoClientOptions = {
  promoteBuffers: false,
  authMechanism: 'SCRAM-SHA-1',
  forceServerObjectId: false,
-  promiseLibrary: Promise,


why did you remove promiseLibrary from these tests? it's deprecated but that shouldn't matter

It was setting the promise lib to something other than null, so it had side effects, even though each MongoClient accepts a promiseLib it modifies it for the process globally

I wouldn't expect that to cause issues for our ts tests though, because they'll never actually run?

Oh sorry yea this one doesn't effect anything, the other one is the runtime usage. We have the explicit type test to check that promiseLibrary is deprecated, I think we can remove this from here now which is just a selection of MongoOptions. (one less search and replace)

leaving this thread open for context but I consider this resolved

test/unit/mongo_client.test.js

src/utils.ts

baileympearson · 2022-09-13T18:03:08Z

src/promise_provider.ts

@@ -4,11 +4,11 @@ import { MongoInvalidArgumentError } from './error';
 const kPromise = Symbol('promise');

 interface PromiseStore {
-  [kPromise]?: PromiseConstructor;
+  [kPromise]: PromiseConstructor | null;


haven't reviewed this file yet - pending team discussion about breaking changes here

src/mongo_client.ts

src/bulk/common.ts

src/mongo_client.ts

baileympearson

most comments addressed/resolved, some questions and suggestions remaining

src/mongo_client.ts

baileympearson · 2022-09-13T20:56:04Z

test/types/community/client.test-d.ts

@@ -35,7 +35,6 @@ const options: MongoClientOptions = {
  promoteBuffers: false,
  authMechanism: 'SCRAM-SHA-1',
  forceServerObjectId: false,
-  promiseLibrary: Promise,


I wouldn't expect that to cause issues for our ts tests though, because they'll never actually run?

test/unit/mongo_client.test.js

src/bulk/common.ts

test/integration/node-specific/custom_promise_library.test.js

src/mongo_client.ts

baileympearson · 2022-09-14T13:44:32Z

test/types/community/client.test-d.ts

@@ -35,7 +35,6 @@ const options: MongoClientOptions = {
  promoteBuffers: false,
  authMechanism: 'SCRAM-SHA-1',
  forceServerObjectId: false,
-  promiseLibrary: Promise,


leaving this thread open for context but I consider this resolved

baileympearson · 2022-09-15T17:59:22Z

src/bulk/common.ts


    if (this.s.executed) {
-      return handleEarlyError(new MongoBatchReExecutionError(), callback);
+      // eslint-disable-next-line @typescript-eslint/require-await


Are we calling maybePromise twice to handle these errors because executeOperation takes a callback? Why not just promisify execute operation?

maybeCallback(async () => { ... return promisify(execute_operation)(....) })

This approach might be preferable as well, because once you make execute_operation return a promise, you won't need to refactor this method much except just to remove the call to promisify.

Wrapping the execOp in maybeCallback (naively, so that's likely why) messes with session allocation because it makes the maybePromise logic run. We can add a note to the refactor exec operation refactor ticket to comeback here, or I can revert the changes here and we can do it altogether.

We'll tackle this once execute_operation has been refactored

test/unit/utils.test.ts

Co-authored-by: Daria Pardue <[email protected]>

nbbeeken · 2022-09-20T17:40:39Z

@baileympearson this is unblocked with the recent release. I've rebased so we can double check we're up to date on changes while we held off on this. (just an FYI no rush at all!)

…odb#3406) Co-authored-by: Daria Pardue <[email protected]>

nbbeeken marked this pull request as draft September 12, 2022 20:19

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch 5 times, most recently from 547b09a to e83ae11 Compare September 12, 2022 22:30

nbbeeken marked this pull request as ready for review September 13, 2022 15:58

nbbeeken requested a review from baileympearson September 13, 2022 15:58

nbbeeken assigned nbbeeken and unassigned nbbeeken Sep 13, 2022

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 13, 2022

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch from e83ae11 to 68eba0d Compare September 13, 2022 16:53

baileympearson requested changes Sep 13, 2022

View reviewed changes

nbbeeken requested a review from baileympearson September 13, 2022 18:38

baileympearson requested changes Sep 13, 2022

View reviewed changes

baileympearson requested changes Sep 14, 2022

View reviewed changes

nbbeeken requested a review from baileympearson September 14, 2022 14:23

nbbeeken mentioned this pull request Sep 14, 2022

fix(NODE-4639): allow PromiseProvider to be null #3412

Merged

5 tasks

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch from a54b2ef to f93126c Compare September 14, 2022 17:23

nbbeeken changed the base branch from main to NODE-4639/promise-provider September 14, 2022 17:23

Base automatically changed from NODE-4639/promise-provider to main September 15, 2022 13:36

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch from a5ad648 to 1643501 Compare September 15, 2022 14:08

baileympearson requested changes Sep 15, 2022

View reviewed changes

nbbeeken added the Blocked Blocked on other work label Sep 15, 2022

dariakp requested changes Sep 15, 2022

View reviewed changes

test/unit/utils.test.ts Outdated Show resolved Hide resolved

test/unit/utils.test.ts Outdated Show resolved Hide resolved

nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 16, 2022

nbbeeken requested a review from dariakp September 16, 2022 17:30

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch from 511ddce to 70b8e76 Compare September 16, 2022 19:03

dariakp previously approved these changes Sep 16, 2022

View reviewed changes

nbbeeken removed the Blocked Blocked on other work label Sep 20, 2022

nbbeeken requested a review from baileympearson September 20, 2022 17:38

nbbeeken and others added 12 commits September 20, 2022 13:39

refactor(NODE-4631): change_stream, gridfs to use maybeCallback

4502578

fix: callback with custom lib

44b6d0c

test: add error case for custom promise

9883042

fixes

fe69c85

lint

c232a19

fix: withSession

3acfb6d

fix: bulk

2ab77fb

lint

894a380

clean up endSessions silencing

f50dc36

rm change in promise_provider

d9b5794

suggestion(daria): fixed!

7b7322e

Co-authored-by: Daria Pardue <[email protected]>

fix: test name

bf668c2

nbbeeken dismissed dariakp’s stale review via bf668c2 September 20, 2022 17:39

nbbeeken force-pushed the NODE-4631/refactor-change-stream-and-gridfs branch from 70b8e76 to bf668c2 Compare September 20, 2022 17:39

baileympearson approved these changes Sep 21, 2022

View reviewed changes

baileympearson merged commit 8c63447 into main Sep 21, 2022

baileympearson deleted the NODE-4631/refactor-change-stream-and-gridfs branch September 21, 2022 14:26

nbbeeken mentioned this pull request Sep 26, 2022

refactor(NODE-4633): executeOperation to use maybeCallback #3421

Merged

5 tasks

ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022

refactor(NODE-4631): change_stream, gridfs to use maybeCallback (mong…

95b605e

…odb#3406) Co-authored-by: Daria Pardue <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406

refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406

nbbeeken commented Sep 12, 2022 •

edited

Loading

baileympearson left a comment

baileympearson Sep 13, 2022

nbbeeken Sep 13, 2022

baileympearson Sep 13, 2022

nbbeeken Sep 13, 2022

baileympearson Sep 14, 2022

baileympearson Sep 13, 2022

baileympearson left a comment

baileympearson Sep 13, 2022

baileympearson Sep 14, 2022

baileympearson Sep 15, 2022

nbbeeken Sep 15, 2022

baileympearson Sep 15, 2022

nbbeeken commented Sep 20, 2022

refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406

refactor(NODE-4631): change_stream, gridfs to use maybeCallback #3406

Conversation

nbbeeken commented Sep 12, 2022 • edited Loading

Description

What is changing?

Double check the following

baileympearson 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

Choose a reason for hiding this comment

baileympearson 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

nbbeeken commented Sep 20, 2022

nbbeeken commented Sep 12, 2022 •

edited

Loading