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

refactor: Move source map lookups to core #14274

Merged
merged 7 commits into from
Apr 15, 2022

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Apr 14, 2022

This had been blocking lots of other cleanups as well. Namely the following transformations gradually faced by JsError have all been moved up front to JsError::from_v8_exception(): finding the first non-deno: source line [old code]; moving JsError::script_resource_name etc. into the first error stack in case of syntax errors [old code]; source mapping JsError::script_resource_name etc. when wrapping the error even though the frame locations are source mapped earlier [old code]; removing JsError::{script_resource_name,line_number,start_column,end_column} entirely in favour of js_error.frames.get(0). We also no longer pass a js-side callback to core/02_error.js from cli. I avoided doing this on previous occasions because the source map lookups were in an awkward place.

core/bindings.rs Outdated Show resolved Hide resolved
core/error.rs Outdated Show resolved Hide resolved
core/error.rs Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, I like how much code is getting removed. That said, I want to get some more eyes on it, as moving source mapping to core might be a bit controversial for other team members.

@bartlomieju bartlomieju merged commit 8b31fc2 into denoland:main Apr 15, 2022
@nayeemrmn nayeemrmn deleted the source-map-core branch April 15, 2022 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants