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

Added WebAssembly.Global #2595

Merged
merged 5 commits into from
Jun 22, 2021
Merged

Added WebAssembly.Global #2595

merged 5 commits into from
Jun 22, 2021

Conversation

syrusakbary
Copy link
Contributor

Added WebAssembly.Global

///
/// [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Global)
#[wasm_bindgen(constructor, js_namespace = WebAssembly, catch)]
pub fn new(global_descriptor: &Object, value: Option<usize>) -> Result<Global, JsValue>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure if here value should be Option<f64> instead

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Object here should probably be a GlobalDescriptor dictionary with typed field accesses, and value should be JsValue most likely to support externref globals.

Copy link
Contributor Author

@syrusakbary syrusakbary Jun 22, 2021

Choose a reason for hiding this comment

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

Done. I set value to Option<&JsValue> &JsValue.

For the global_descriptor I used the same pattern as WebAssembly.Table or WebAssembly.Memory: using Object.

I do agree that having a GlobalDescriptor dictionary would be safer, but I'm not clear on how to do it looking at the code. Any tips here would be appreciated.

PS: perhaps we can improve the Global, Table and Memory constructors to take a proper descriptor type instead of an Object in a PR apart?

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I think CI is failing?

///
/// [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Global)
#[wasm_bindgen(constructor, js_namespace = WebAssembly, catch)]
pub fn new(global_descriptor: &Object, value: Option<usize>) -> Result<Global, JsValue>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Object here should probably be a GlobalDescriptor dictionary with typed field accesses, and value should be JsValue most likely to support externref globals.

crates/js-sys/src/lib.rs Show resolved Hide resolved
@syrusakbary
Copy link
Contributor Author

syrusakbary commented Jun 22, 2021

Tests are failing (Run UI tests more specifically), but I have no idea how to resolve them or why they are failing.

Any help here would be appreciated!

EDIT: I figured it out, was a bit obscure error raising as a side-effect of having one extra implementation. I believe tests shall be passing soon. So the PR should be ready to review!

@alexcrichton alexcrichton merged commit e4ab260 into rustwasm:master Jun 22, 2021
@alexcrichton
Copy link
Contributor

Thanks!

@Hywan Hywan mentioned this pull request Jul 14, 2021
11 tasks
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.

2 participants