diagnostics_channel: return original thenable

[Qard]

This makes tracePromise return the original thenable to allow custom thenable types to retain their methods rather than producing the chained result type.

This branches the behaviour between native promises and thenables such that native promises retain the correct unhandledRejection behaviour while thenables produce the correct original return value and so retain methods present on that type.

I think that because native promises are consistently shaped it doesn't actually matter that it remains chained rather than branched.

cc @nodejs/diagnostics

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.77%. Comparing base (efa05f2) to head (3522e89).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #62407   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         697      697           
  Lines      215773   215782    +9     
  Branches    41304    41300    -4     
=======================================
+ Hits       193704   193713    +9     
+ Misses      14162    14158    -4     
- Partials     7907     7911    +4     

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	98.16% <100.00%> (+0.02%)	⬆️

... and 32 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Renegade334]

Does this change have a specific case in mind? The A+ standard mandates .then() to return another "promise" instance (either the same object or a new object at the implementation's discretion), so any custom features of a compliant thenable should also be present on the object returned by .then()?

[Qard]

No, that is not the case. A thenable can return any variety of promise, not necessarily another of itself. It's most common that a thenable returns native promises from its then/catch, so you lose access to whatever methods it had when following the chain.

[Renegade334]

Nit: also having onResolve for consistency

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72269/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72277/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/72280/

[logaretm]

I ran into this while working on TracingChannel proposals for several libraries. I think Prisma and pg both have a concrete real-world case.

Prisma Client operations return a lazy thenable with fluent relation-chaining methods (e.g. prisma.user.findUnique(...).posts())

then(onFulfilled, onRejected) {
  return _callback().then(onFulfilled, onRejected)
}

So when tracePromise chains .then() on it, the return value is a native Promise and all PrismaPromise-specific methods are lost:

const query = operationChannel.tracePromise(
  () => prisma.user.findUnique({ where: { id: 1 } }),
  context
);
// query is now a native Promise
// query.posts() → TypeError: query.posts is not a function
// query.requestTransaction() → gone

A potential workaround is to avoid tracePromise for these cases and use traceCallback instead, which adds complexity. This fix should make it possible to use tracePromise directly.

---

We also hit this in the TracingChannel PR for node-postgres (brianc/node-postgres#3650).

pg supports a user-configurable Promise constructor via https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/client.js#L12. Which again was worked around with traceCallback instead.