Skip to content

diagnostics_channel: return original thenable#62407

Open
Qard wants to merge 1 commit intonodejs:mainfrom
Qard:trace-promise-return-original-thenable
Open

diagnostics_channel: return original thenable#62407
Qard wants to merge 1 commit intonodejs:mainfrom
Qard:trace-promise-return-original-thenable

Conversation

@Qard
Copy link
Member

@Qard Qard commented Mar 23, 2026

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

@Qard Qard self-assigned this Mar 23, 2026
@Qard Qard added confirmed-bug Issues with confirmed bugs. diagnostics_channel Issues and PRs related to diagnostics channel labels Mar 23, 2026
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 23, 2026
This makes tracePromise return the original thenable to allow custom
thenable types to retain their methods rather than producing the
chained result type.
@Qard Qard force-pushed the trace-promise-return-original-thenable branch from fc213c4 to 3eb07ea Compare March 23, 2026 14:41
@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.69%. Comparing base (2263b4d) to head (3eb07ea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #62407   +/-   ##
=======================================
  Coverage   89.68%   89.69%           
=======================================
  Files         676      676           
  Lines      206710   206720   +10     
  Branches    39584    39585    +1     
=======================================
+ Hits       185398   185417   +19     
- Misses      13441    13442    +1     
+ Partials     7871     7861   -10     
Files with missing lines Coverage Δ
lib/diagnostics_channel.js 98.52% <100.00%> (+0.03%) ⬆️

... and 28 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.

Copy link
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

Comment on lines +404 to +405
// TODO(qard): Not realm safe? Should be a construct.name check?
if (result.constructor === Promise) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require('internal/util/types').isPromise()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confirmed-bug Issues with confirmed bugs. diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants