feat(NODE-2014)!: return the result of provided callback in withTransaction and withSession

[durran]

Description

Updates withTransaction and withSession helpers to return the return value of the user provided callback.

What is changing?

• session.withTransaction() now properly returns the return value of the callback.
• client.withSession() updates its signature to return Promise<any> instead of Promise<void and returns the return value of the provided callback.
• Updates migration guide.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-2014

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
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[durran]

This has nothing to do with this ticket specifically, but was failing the lint task on CI.

[nbbeeken]

Actual changes LGTM, but outstanding question about our approach here

[nbbeeken]

The docs need a little update too:

   * @remarks
   * This function:
   * - Will return the result of the withTransaction callback if every operation is successful
   * - Will return `undefined` if the transaction is intentionally aborted with `await session.abortTransaction()`
   * - Will throw if one of the operations throws or `throw` statement is used inside the `withTransaction` callback

Which makes me question, should we require that the callback return something truthy (via TS) or default to something truthy (maybe true) if the callback returns undefined? Otherwise, how can someone differentiate an intentionally aborted transaction and a committed one if they don't have a return value?

Spec ref

[durran]

I think returning true sounds good if they haven't returned anything.

[nbbeeken]

Sounds good to me, I'm thinking we can just add value ?? true (this would also cover return null, but I think that's ok?) and a test for that case along with docs. I'll update the ticket AC just so we keep track

[addaleax]

Will it still be possible to get the legacy return value somehow (i.e. the Document returned by session.commitTransaction())?

I think it would be an acceptable breaking change in mongosh if that goes away (right now our own withTransaction helper returns it), but it might be nice to avoid this.

To avoid this, you could do some hackery to attach this secondary result to the returned Promise itself or return a tuple, e.g.

• withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<T> & { transactionResult: Promise<Document | undefined> }
• withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<[callbackResult: T, transactionResult: Document|undefined]>

(that would also maybe solve the problem mentioned above about distinguishing the callback returning undefined from an aborted transaction?)

[durran]

My only worry here is with this change withTransaction and withSession now diverge, and one of the goals of the ticket was to keep those two APIs the same for consistency. But I'm flexible.

[addaleax]

Maybe it could just be a separate function or item in options then that makes it return a tuple?

[nbbeeken]

From slack: We're moving this effort down the road to the next major to get ahead of documentation updates that we would want to have ready before committing to this change.