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). |
This might be an acceptable compromise, so long as it doesn't blow up our bundle size too much. 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.
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. |
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