-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
errors: annotate frames for prepareStackTrace() #30984
Conversation
I'm very uncomfortable adding more surface to this API. I'd prefer something not attached to any JS/engine built-ins. |
While I share @devsnek's concerns in general, it seems the interop between I am not sure whether what's proposed in this PR should be the way forward, though...it seems at the moment, the least intrusive first step would be to implement what @targos suggested in #29994 (comment)
(though in practice there's no original function - it's either undefined, or set by the user). That is, instead of having |
@joyeecheung @devsnek I think it would be extremely valuable to upstream tooling to expose the applied source-map information, in the form of an annotated stack trace. I can understand why folks want to override I was picturing that we get feedback from folks on the V8 side of things, and make sure that, if we extend this API surface, it's on their radar. |
Alternative approachI could imagine an alternative where we expose Should Error.prepareStackTrace override source-maps?
I don't think this is enough for libraries in the wild. I looked at why Given that express is one of the most popular deployment environments for Node.js, and would benefit from user friendly stack traces (with the original source position). I think figuring out an API to support them would be a great litmus test for the design of the source-map feature. User testingThis is particularly on my mind, because I asked @iansu to try out source-maps at Node interactive, and the first thing he did was start an express app with |
How does it work without --enable-source-maps? Just stack traces but with only the actual source positions instead of original source positions? IIUC depd would still have to be updated even with this change to display the original source positions. It also has to detect Node.js versions in the code to make this work with multiple Node.js versions. Having an array with objects whose methods are uncertain do not seem to result in very clean code in that case - I guess an alternative would be to pass a second array to the hook function that has source-mapped call sites, instead of mutating the original one. This probably also works better when there are fewer call sites in the original code (e.g. when one line of code is mapped to many lines of polyfills / transpiled code). |
correct 👍 I would like to:
If we took this approach, I think this would make the upstream algorithm a bit of a hassle, because you tend to have source-maps for some of the stack trace, but not other parts. And, as in our solution, you might want to show both the original and remapped position ... If we were going to go down the path of adding a parameter to |
i think we should enable prepareStackTrace using the regular trace (no modified or inserted call sites) in the short term. This will make all the weird libs that depend on it continue to function. Longer term we can then explore new apis, modifications to the api, etc, that will let source maps play better with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m good with this, personally.
I am not sure whether it actually comes up in real world, but isn't it also possible that there might be multiple original frames that only correspond to one runtime frame? (e.g. if the transpiler "inlines" some calls as an optimization). In that case how would the user get this information out of the mutated array? |
@joyeecheung a source map can map from one transpiled file, to many original files, but I don't think that a call site could be in more than one of these files? I like the suggestion of supplementing the API with an additional parameter that allows the user to lookup, rather than annotating the object that originates in V8. I'll work on a PR over the next few days that adds back support for overriding This would potentially allow us to support edge-cases like you're suggesting, by returning an array. |
When source-maps are enabled, overriding Error.prepareStackTrace() is again supported. Error.prepareStackTrace() will now be passed stack frames with the methods getOriginalFileName(), getOriginalLineNumber(), and getOriginalColumnNumber() which provide information parsed from source map.
3bc82ac
to
0521b21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the reservations but I believe this makes node more holistic for developers.
@devsnek @joyeecheung - do either of you feel strongly enough to block? I noticed this currently has 2 approvals and I wanted to make sure that if you wish to object you do :] |
Do we need to provide a WeakMap through the prepareStackTrace API? Can we just expose a function that allows the user to get the mapping out-of-band? Judging from how CoffeScript maps the stack I think this would already be enough:
Maybe some insight from people working on other transpiled languages would be useful here, but from what I can tell mutating the callsites do not seem to be necessary for this kind of support, then I'd like to avoid this if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put my -1 here unless the mutation to the callsites is proven necessary (at the moment I believe it's not)
I like the idea of exposing an out of band API for looking up source maps, which upstream tooling could use. I'll work on drafting something up soon; I think we can pretty much just expose the API we already have internally. |
By the way, I think it might still be possible to provide in-band information in the callback that's useful enough for users if source maps supports "stack tracing mapping without knowledge of the source language" - which does not seem to be specified at the moment? I am pretty ignorant about the source map standardization so I can't tell if that's a wont-fix or a TODO. |
@bcoe are you going to put up a pr to re-enable unmodified prepareStackTrace? If not I might be able to get around to it. |
@devsnek I'll have a PR within a few hours that exposes |
When source-maps are enabled, overriding
Error.prepareStackTrace()
is again supported.
Error.prepareStackTrace()
will now be passedstack frames with the methods
getOriginalFileName()
,getOriginalLineNumber()
, andgetOriginalColumnNumber()
which provideinformation parsed from source map.
I think this is a good solution to the exception @dougwilson raises in #29994, and provides tools like depd an option for taking advantage of source-map information.
fixes #29994
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes