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

Add extends = ... attributes to js-sys types #670

Closed
44 tasks done
fitzgen opened this issue Aug 8, 2018 · 3 comments
Closed
44 tasks done

Add extends = ... attributes to js-sys types #670

fitzgen opened this issue Aug 8, 2018 · 3 comments
Labels
good first issue This is a good issue for people who have never contributed to wasm-bindgen before help wanted We could use some help fixing this issue! js-sys Issues related to the `js-sys` crate

Comments

@fitzgen
Copy link
Member

fitzgen commented Aug 8, 2018

We should add all the inheritance relations for global JS types to js-sys.

We can use this snippet of JS to log the prototype chain for something, eg Uint8Array:

function logPrototypeChain(cls) {
  let obj = cls.prototype;
  do {
    console.log(obj.constructor.name);
    obj = obj.__proto__;
  } while (obj != null);
}

logPrototypeChain(Uint8Array);

Pasting that snippet in a browser's devtools console logs:

Uint8Array
TypedArray
Object

So we would add #[wasm_bindgen(extends = Object, extends = TypedArray) to Uint8Array's import definition and a corresponding test to the appropriate file in crates/js-sys/tests/.. like:

use wasm_bindgen::JsCast;

#[wasm_bindgen_test]
fn test_blah_inheritance() {
    let blah = js_sys::Blah::new(); // its constructor might require different args

    // Assert JS actually agrees with our set of base classes.
    assert!(blah.is_instance_of::<Blah>());
    assert!(blah.is_instance_of::<BlahBase>());
    // etc for each base class...
    assert!(blah.is_instance_of::<js_sys::Object>());

    // Ensure that we generated the appropriate `AsRef` upcasting implementations
    // for each base class.
    let _: &BlahBase = blah.as_ref();
    // etc for each base class...
    let _: &js_sys::Object = blah.as_ref();
}

Here is an example PR for the RegExp type: #671

See also js-sys contributing documentation: https://rustwasm.github.io/wasm-bindgen/js-sys.html


Here are all the JS global import types for which we need to add this information:

  • Array
  • ArrayBuffer
  • Boolean
  • DataView
  • Date
  • Error
  • EvalError
  • Float32Array
  • Float64Array
  • Function
  • Generator
  • Int16Array
  • Int32Array
  • Int8Array
  • Intl.Collator
  • Intl.DateTimeFormat
  • Intl.NumberFormat
  • Intl.PluralRules
  • JSON
  • Map
  • Math
  • Number
  • Promise
  • RangeError
  • ReferenceError
  • Reflect
  • RegExp
  • Set
  • String
  • Symbol
  • SyntaxError
  • TypeError
  • TypedArray
  • URIError
  • Uint16Array
  • Uint32Array
  • Uint8Array
  • Uint8ClampedArray
  • WeakMap
  • WeakSet
  • WebAssembly.Module
  • WebAssembly.Instance
  • WebAssembly.Table
  • WebAssembly.Memory
@fitzgen fitzgen added help wanted We could use some help fixing this issue! good first issue This is a good issue for people who have never contributed to wasm-bindgen before Blocking Rust 2018 js-sys Issues related to the `js-sys` crate labels Aug 8, 2018
fitzgen added a commit to fitzgen/wasm-bindgen that referenced this issue Aug 8, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Aug 8, 2018

Here is an example of adding extends to the RegExp type: #671

@kraai
Copy link
Contributor

kraai commented Aug 11, 2018

It seems like the extends attribute isn't necessary for Promise. The following test passes without any changes to lib.rs:

use wasm_bindgen_test::*;                                                       
use wasm_bindgen::JsCast;                                                       
use js_sys::*;                                                                  
                                                                                
#[wasm_bindgen_test]                                                            
fn test_promise_inheritance() {                                                 
    let promise = Promise::new(&mut |_, _| ());                                 
    assert!(promise.is_instance_of::<Promise>());                               
    assert!(promise.is_instance_of::<Object>());                                 
}                                                                               

Should it be marked as complete, should the test be added, or should the extends attribute and test be added anyway?

kraai added a commit to kraai/wasm-bindgen that referenced this issue Aug 11, 2018
@fitzgen
Copy link
Member Author

fitzgen commented Aug 13, 2018

Good catch! The example test I provided only tests the dynamic JS behavior with instanceof, which doesn't exercise the code emitted by extends = ... at all :-p Instead we should do something like this:

#[wasm_bindgen_test]
fn test_promise_inheritance() {
    let promise = Promise::new(&mut |_, _| ());

    // Assert that JS matches our inheritance hierarchy.
    assert!(promise.is_instance_of::<Promise>());
    assert!(promise.is_instance_of::<Object>());

    // Ensure that we emitted the appropriate `AsRef` implementations for each base class.
    let _: &Object = promise.as_ref();
}

That bottom bit is the new bits, and that should fail to compile without the extends = ... attributes!

kraai added a commit to kraai/wasm-bindgen that referenced this issue Aug 16, 2018
kraai added a commit to kraai/wasm-bindgen that referenced this issue Aug 17, 2018
kraai added a commit to kraai/wasm-bindgen that referenced this issue Aug 17, 2018
kraai added a commit to kraai/wasm-bindgen that referenced this issue Aug 17, 2018
fitzgen added a commit to fitzgen/wasm-bindgen that referenced this issue Sep 6, 2018
fitzgen added a commit to fitzgen/wasm-bindgen that referenced this issue Sep 6, 2018
fitzgen added a commit to fitzgen/wasm-bindgen that referenced this issue Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue This is a good issue for people who have never contributed to wasm-bindgen before help wanted We could use some help fixing this issue! js-sys Issues related to the `js-sys` crate
Projects
None yet
Development

No branches or pull requests

2 participants