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

Do not convert exceptions to JSON and back #4214

Merged
merged 2 commits into from
Mar 2, 2020
Merged

Do not convert exceptions to JSON and back #4214

merged 2 commits into from
Mar 2, 2020

Conversation

piscisaureus
Copy link
Member

No description provided.

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
4d8a0b5
@piscisaureus
Copy link
Member Author

048_media_types_jsx is super flaky like always

@piscisaureus piscisaureus changed the title WIP: Do not convert exceptions to JSON and back Do not convert exceptions to JSON and back Mar 2, 2020
bartlomieju
bartlomieju approved these changes Mar 2, 2020
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 simplification

ry
ry reviewed Mar 2, 2020
View reviewed changes
core/js_errors.rs Show resolved Hide resolved
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
df2e419
@ry
Copy link
Member

ry commented Mar 2, 2020

One more thing - can you check that this comment still makes sense (and update if not)

deno/cli/op_error.rs

Lines 3 to 18 in 4a47ffa

//! There are many types of errors in Deno:
//! - ErrBox: a generic boxed object. This is the super type of all
//! errors handled in Rust.
//! - JSError: exceptions thrown from V8 into Rust. Usually a user exception.
//! These are basically a big JSON structure which holds information about
//! line numbers. We use this to pretty-print stack traces. These are
//! never passed back into the runtime.
//! - OpError: these are errors that happen during ops, which are passed
//! back into the runtime, where an exception object is created and thrown.
//! OpErrors have an integer code associated with them - access this via the `kind` field.
//! - Diagnostic: these are errors that originate in TypeScript's compiler.
//! They're similar to JSError, in that they have line numbers.
//! But Diagnostics are compile-time type errors, whereas JSErrors are runtime exceptions.
//!
//! TODO:
//! - rename/merge JSError with V8Exception?

ry
ry approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Very nice clean up

@piscisaureus
Copy link
Member Author

Hitting a wall while trying to remove ErrWithV8Handle type.
So I'm going to land this first before attempting further clean-ups.

piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
ba1eb6f
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
6789f12
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Don't reset exception handle after calling ErrWithV8Handle::get_handl…
62360d4 
…e() (denoland#4214)
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
a03107f
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
0565a04
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Mar 2, 2020
@piscisaureus
Do not convert exceptions to JSON and back (denoland#4214)
853000d
@piscisaureus piscisaureus merged commit eafd40f into denoland:master Mar 2, 2020
@piscisaureus piscisaureus deleted the exc branch March 2, 2020 22:56
dubiousjim added a commit to dubiousjim/deno that referenced this pull request Mar 5, 2020
@dubiousjim
Merge remote-tracking branch 'denoland/master' into filesystem2a
cea3d72 
* denoland/master: (22 commits)
  fix event target tests
  Support async function and EventListenerObject as listeners (denoland#4240)
  Allow BadResource errors to take a custom message (denoland#4251)
  Document TypeScript compiler options (denoland#4241)
  refactor: preliminary cleanup of Deno.runTests() (denoland#4237)
  refactor: cleanup compiler runtimes (denoland#4230)
  Use discord instead of gitter (denoland#4253)
  Remove unnecessary macro from cli/ops/tty.rs (denoland#4254)
  Remove Deno.errors.Other (denoland#4249)
  refactor: rewrite testPerm into unitTest (denoland#4231)
  Migrate internal bundles to System (denoland#4233)
  Fix inlining of lib.dom.iterable.d.ts. (denoland#4242)
  Fix `deno install` file name including extra dot on Windows (denoland#4243)
  assert build success for test plugin (denoland#4223)
  Disable flaky and broken tests (denoland#4239)
  add assertOps sanitizer in cli/js/ unit tests (denoland#4209)
  misc: reduce unnecesarry output in cli/js tests (denoland#4182)
  feat(std/node): add directory classes (denoland#4087)
  Do not convert exceptions to JSON and back (denoland#4214)
  Don't reset exception handle after calling ErrWithV8Handle::get_handle() (denoland#4214)
  ...
mhvsa pushed a commit to mhvsa/deno that referenced this pull request Mar 6, 2020
@piscisaureus @mhvsa
Don't reset exception handle after calling ErrWithV8Handle::get_handl…
918086c 
…e() (denoland#4214)
mhvsa pushed a commit to mhvsa/deno that referenced this pull request Mar 6, 2020
@piscisaureus @mhvsa
Do not convert exceptions to JSON and back (denoland#4214)
6a79fce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

@bartlomieju bartlomieju bartlomieju approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@piscisaureus @ry @bartlomieju