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.

[Renegade334]

This doesn't apply to custom native Promises, so these cases probably need to be handled differently.

process.on('unhandledRejection', e => console.error('Unhandled rejection:', e))

class MyPromise extends Promise {}
new MyPromise((resolve, reject) => reject(new Error()))

// Unhandled rejection: Error
//     at ...

Could either abort the prototype check and just return result.then(onResolve, onRejectWithRethrow) for all native Promises, or keep the PromisePrototypeThen primordial path for vanilla Promises and have a result.then()-returning path for native Promises that fail the prototype check.

[Qard]

That's intentional. Non-extended native promises are the only variety of promise that you can safely know that chaining will not break the type contract. The specific problem this is trying to address is that many custom promise types are (wrongly) created by sub-classing Promise but then overriding the then(...) method to return a different promise, orphaning the sub-classed promise instance in the process. Both OpenAI and Anthropic API clients do this. I'm pretty sure at least one of the API client generation as a product vendors are actually doing this and so this pattern exists in many, many API client libraries.

This seems to me to be the least-bad approach as there's not really any reason one would sub-class a promise unless they intended to actually do something custom with it, in which case we should be returning that custom thing or we are breaking their API contract.