fix(core): Return same value from startSpan as callback returns#19300
fix(core): Return same value from startSpan as callback returns#19300
startSpan as callback returns#19300Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Results 📊Generated by Codecov Action |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
Converting to a draft, going to push some more commits to try out a slightly different approach to this. The Remix failures are interesting. Playwright timing out is less interesting, but also could be indicative of a problem, if it's waiting for an expected failure that we're swallowing (haven't dug deeply into it yet). |
isaacs
force-pushed
the
sig/promise-startspan-types
branch
from
58c695d to
a729eb8
Compare
|
So, there is no way to return the actual promise and also track the error, without obscuring the Added a commit to copy any new properties that exist on the original promise, onto the chained one. If the property is a function, then a wrapper function is added that calls the original method, bound to the original object. This does still have the slight gotcha that any add-on methods on the original promise are now bound to it. So, this might be surprising: const original = getDecoratedPromiseSomehow();
const returned = startSpan({..}, () => original);
const newObject = {};
returned.addedMethod.call(newObject); // surprise! runs in this-context of `original`We can detect this and work around it easily enough if needed, but I'm thinking this is already probably more than enough support for this edge case, and if we're going any further, sniffing the Will check back in a bit to see what CI thinks of this. |
isaacs
force-pushed
the
sig/promise-startspan-types
branch
from
a729eb8 to
77f8f02
Compare
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Deps
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
isaacs
force-pushed
the
sig/promise-startspan-types
branch
3 times, most recently
from
cf08b2f to
44ae312
Compare
Copy properties from the original promise onto the chained tracker, so that we can return something that can inspect the error, does not obscure `unhandledrejection`, *and* supports jQuery's extended PromiseLike objects. We only do this if the original PromiseLike object or its chained counterpart are not `instanceof Promise`. So far, we have not encountered such decoration on "normal" Promise objects, so this is a fast way to ensure that we're only providing this affordance where it's needed. This does introduce a limitation that if someone decorates a standard Promise object, and its decorations do not extend to chained results of `then()`, then we will lose them. If and when that happens, we can consider extending this check to something more thorough, even if it's slightly less performant. A guard is added to prevent cases where a chained/copied promise is passed through this function again, so that we know we still have to do the operation even though it's a "normal" Promise.
isaacs
force-pushed
the
sig/promise-startspan-types
branch
from
44ae312 to
9b8d764
Compare
|
@isaacs took over this PR, so I cannot approve it "officially" in the UI on GitHub but looks good to me 👍 |
2 participants
When using
startSpan,.then()is called on the callback's return value. This creates another Promise which is a problem in for example jQuery, where the returned Promise does not have the same methods as the jqXHR object.Closes #19242