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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove PrettyJsError and js_error_create_fn #14378

Merged
merged 19 commits into from
Apr 26, 2022
Merged

refactor: Remove PrettyJsError and js_error_create_fn #14378

merged 19 commits into from
Apr 26, 2022

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Apr 23, 2022 •
edited
Loading

Blocked on #14394.
Fixes #14377. Also adds pretty-printed errors and source mapping for deno compiled executables.

bartlomieju
bartlomieju reviewed Apr 25, 2022
View reviewed changes
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.

There's a lot going on in this PR @nayeemrmn, would it be possible to split it into two separate PRs? One for changes to core/ and the other one that updates workers and compile subcommand

cli/main.rs Show resolved Hide resolved
cli/tests/integration/compile_tests.rs Outdated Show resolved Hide resolved
core/modules.rs Outdated Show resolved Hide resolved
@nayeemrmn
Copy link
Collaborator Author

There's a lot going on in this PR @nayeemrmn, would it be possible to split it into two separate PRs? One for changes to core/ and the other one that updates workers and compile subcommand

I can split it it into one for removing ErrWithV8Handle (core-only refactor) and one for removing PrettyJsError along with create_js_error_fn from core's API.

@bartlomieju
Copy link
Member

There's a lot going on in this PR @nayeemrmn, would it be possible to split it into two separate PRs? One for changes to core/ and the other one that updates workers and compile subcommand

I can split it it into one for removing ErrWithV8Handle (core-only refactor) and one for removing PrettyJsError along with create_js_error_fn from core's API.

That sounds good 👍

@nayeemrmn nayeemrmn changed the title refactor: Remove wrappers around JsError refactor: Remove PrettyJsError and js_error_create_fn Apr 25, 2022
@bartlomieju bartlomieju mentioned this pull request Apr 26, 2022
Closed
bartlomieju
bartlomieju approved these changes Apr 26, 2022
View reviewed changes
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, great refactor @nayeemrmn

@bartlomieju bartlomieju merged commit 9853c96 into denoland:main Apr 26, 2022
@nayeemrmn nayeemrmn deleted the remove-js-error-wrappers branch April 26, 2022 23:19
@bartlomieju bartlomieju mentioned this pull request Apr 27, 2022
Closed
crowlKats pushed a commit that referenced this pull request Apr 28, 2022
@nayeemrmn @crowlKats
refactor: Remove PrettyJsError and js_error_create_fn (#14378)
b7d63ea 
This commit:
- removes "fmt_errors::PrettyJsError" in favor of "format_js_error" fn
- removes "deno_core::JsError::create" and 
"deno_core::RuntimeOptions::js_error_create_fn"
- adds new option to "deno_runtime::ops::worker_host::init"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

@dsherret dsherret Awaiting requested review from dsherret

@AaronO AaronO Awaiting requested review from AaronO

@kt3k kt3k Awaiting requested review from kt3k

@piscisaureus piscisaureus Awaiting requested review from piscisaureus

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Worker error events missing location info
2 participants
@nayeemrmn @bartlomieju