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

Web API: EventTarget implementation #2377

Merged
merged 8 commits into from May 27, 2019

Conversation

2 participants
@acconrad
Copy link
Contributor

commented May 19, 2019

This PR implements the EventTarget Web API spec.

One big decision that @ry and co will need to make is around implementing the Node standard as a tree. The event target implementation is a beast but it makes a lot of assumptions that there is this base Node object which, according to this implementation of JS DOM encompasses way more than I believe #1047 bargained for.

So in effort to get a more robust implementation in, I commented some things out and left some TODOs in place but there's a lot to consider.

acconrad added some commits May 19, 2019

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Not sure if my builds are failing because of this:

[1/38] ACTION //core:deno_rustc(//build/toolchain/mac:clang_x64)
FAILED: rust_crates/libdeno.rlib
python ../../build_extra/rust/run.py /Users/me/Code/deno/prebuilt/mac/sccache rustc ../../core/lib.rs --crate-name=deno --crate-type=rlib --emit=link,dep-info --edition=2018 --out-dir=rust_crates -Cextra-filename= -Cmetadata=\"_rustc\ 1.31.1\ \(b6c32da9b\ 2018-12-18\)\" -L dependency=rust_crates --color=always -g -Dwarnings --extern futures=rust_crates/libfutures.rlib --extern libc=rust_crates/liblibc.rlib --extern serde_json=rust_crates/libserde_json.rlib --extern log=rust_crates/liblog.rlib
error[E0658]: imports can only refer to extern crate names passed with `--extern` on stable channel (see issue #53130)
   --> ../../core/libdeno.rs:143:9
    |
90  | / pub struct PinnedBufRaw {
91  | |   data_ptr: *mut u8,
92  | |   data_len: usize,
93  | |   pin: *mut c_void,
94  | | }
    | |_- not an extern crate passed with `--extern`
...
143 |   pub use PinnedBufRaw as deno_pinned_buf;
    |           ^^^^^^^^^^^^
    |
note: this import refers to the struct defined here
   --> ../../core/libdeno.rs:90:1
    |
90  | / pub struct PinnedBufRaw {
91  | |   data_ptr: *mut u8,
92  | |   data_len: usize,
93  | |   pin: *mut c_void,
94  | | }
    | |_^

error: aborting due to previous error
@ry

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

@acconrad Your local build - right? CI doesn't seem to be failing with that.

Check that you're using rust 1.34.1 and that your submodules are all updated. Make sure you're rebased on master.

CI is oddly failing with

---- js_errors::tests::js_error_apply_source_map_2 stdout ----
thread 'js_errors::tests::js_error_apply_source_map_2' panicked at 'assertion failed: actual.frames[0].script_name.ends_with("js/util.ts")', ../../cli\js_errors.rs:398:5
@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Alright @ry I was able to reproduce this locally now that I've upgraded rust, here's the full stack trace from the error we're seeing on CI. I did notice something funny though from the actual Rust test that this is breaking on:

deno/cli/js_errors.rs

Lines 396 to 398 in 22feb74

// Because this is accessing the live bundle, this test might be more fragile
assert_eq!(actual.frames.len(), 1);
assert!(actual.frames[0].script_name.ends_with("js/util.ts"));

It certainly looks like it's known to be brittle. Here's that stack trace:

failures:

---- js_errors::tests::js_error_apply_source_map_2 stdout ----
thread 'js_errors::tests::js_error_apply_source_map_2' panicked at 'assertion failed: actual.frames[0].script_name.ends_with("js/util.ts")', ../../cli/js_errors.rs:398:5
stack backtrace:
   0:        0x103b56b13 - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::h2e1a54aafa1209de
   1:        0x103b4f752 - std::sys_common::backtrace::_print::h670ed14fc4950210
   2:        0x103b530b6 - std::panicking::default_hook::{{closure}}::h5d2e6205d52978de
   3:        0x103b52c1e - std::panicking::default_hook::h8e6307de267f5f56
   4:        0x103b5382f - std::panicking::rust_panic_with_hook::h9bb34120d55c0ca9
   5:        0x1058f11f7 - std::panicking::begin_panic::h98b762b2d3e5c3f2
   6:        0x1061b421c - cli_test_bin::js_errors::tests::js_error_apply_source_map_2::h8e9815dcc728f727
   7:        0x1061b3b20 - cli_test_bin::js_errors::tests::js_error_apply_source_map_2::{{closure}}::h48d4326d76b7e769
   8:        0x105fad280 - core::ops::function::FnOnce::call_once::h0fa00a676e0297cc
   9:        0x103af8241 - <F as alloc::boxed::FnBox<A>>::call_box::h57df398114e3caca
  10:        0x103b6351e - __rust_maybe_catch_panic
  11:        0x103b14ba6 - test::run_test::run_test_inner::{{closure}}::h462d8984151dee76
  12:        0x103af05f4 - std::sys_common::backtrace::__rust_begin_short_backtrace::h3ba2b968d1f10d0e
  13:        0x103af0ca4 - std::panicking::try::do_call::h32a52f1f76578234
  14:        0x103b6351e - __rust_maybe_catch_panic
  15:        0x103af83e4 - <F as alloc::boxed::FnBox<A>>::call_box::hf0fc41817deacef6
  16:        0x103b6232b - std::sys::unix::thread::Thread::new::thread_start::hf63bd47663b691b4
  17:     0x7fff6213d2ea - _pthread_body
  18:     0x7fff62140248 - _pthread_start


failures:
    js_errors::tests::js_error_apply_source_map_2

test result: FAILED. 94 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
js/util.ts Outdated
@@ -207,3 +208,90 @@ export function splitNumberToParts(n: number): number[] {
const higher = (n - lower) / 0x100000000;
return [lower, higher];
}

// Utility functions for DOM nodes

This comment has been minimized.

Copy link
@ry

ry May 23, 2019

Collaborator

I think the error is caused by making changes to js/util.ts. Note the comment:

// Because this is accessing the live bundle, this test might be more fragile 

Furthermore, I don't think these belong here anyway. js/util.ts is more for assert() and other functions that don't have many dependencies. By adding the DOM stuff here you're creating a lot of dependencies to DOM in other modules that have nothing to do with it.

I suggest moving these into js/event_target.ts since it seems like they're not used elsewhere. If you really need a utility module, create something like js/dom_util.ts.

acconrad added some commits May 26, 2019

@acconrad

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

alright @ry finally everything is passing - let me know when you want to get this in

@ry

ry approved these changes May 27, 2019

Copy link
Collaborator

left a comment

LGTM - thanks @acconrad

@ry ry merged commit 9fd4096 into denoland:master May 27, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@acconrad acconrad deleted the acconrad:acc-event-target-impl branch Jun 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.