Skip to content
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

Rewrite "prepareStackTrace" to use Rust API #7644

Closed
bartlomieju opened this issue Sep 23, 2020 · 1 comment
Closed

Rewrite "prepareStackTrace" to use Rust API #7644

bartlomieju opened this issue Sep 23, 2020 · 1 comment
Assignees
Labels
cli related to cli/ dir deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)

Comments

@bartlomieju
Copy link
Member

Currently CLI uses Error.prepareStackTrace API to perform source mapping and formatting of errors.

This situation leads to overall complexity of this method as well as requirement to manually acquire information about stack frames as well as distributing the logic between JS and Rust.

We want to avoid jumping between JS and Rust as much as possible and to solve this problem in this situation we need to:

  1. Add new APIs to V8's StackFrame to acquire the same data as using CallSite:

function evaluateCallSite(callSite) {
return {
this: callSite.getThis(),
typeName: callSite.getTypeName(),
function: callSite.getFunction(),
functionName: callSite.getFunctionName(),
methodName: callSite.getMethodName(),
fileName: callSite.getFileName(),
lineNumber: callSite.getLineNumber(),
columnNumber: callSite.getColumnNumber(),
evalOrigin: callSite.getEvalOrigin(),
isToplevel: callSite.isToplevel(),
isEval: callSite.isEval(),
isNative: callSite.isNative(),
isConstructor: callSite.isConstructor(),
isAsync: callSite.isAsync(),
isPromiseAll: callSite.isPromiseAll(),
promiseIndex: callSite.getPromiseIndex(),
};
}

  1. Remove prepareStackTrace method from cli/rt/40_error_stack.js and instead use Isolate::set_prepare_stack_trace_callback - needs to be added to rusty_v8 first

CC @ry @nayeemrmn

@bartlomieju
Copy link
Member Author

With recent improvements to error handling by @nayeemrmn done in #14378, #14394, #14302 and #14274 I feel this issue is no longer of high importance. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir deno_core Changes in "deno_core" crate are needed feat new feature (which has been agreed to/accepted)
Projects
None yet
Development

No branches or pull requests

2 participants