-
Notifications
You must be signed in to change notification settings - Fork 181
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
wasm-bindgen: Added support for Internet Explorer 11 #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @zer0x64 for this patch. If Rust docs reccomend wasm2js, and supporting IE isn't that hard, we should do it. Note, we would probably not support IE versions before 11.
Quick question, is there an easy way to test this in CI? I don't think so, but I thought I would ask.
src/wasm32_bindgen.rs
Outdated
|
||
// Internet Explorer 11 | ||
#[wasm_bindgen(method, getter, js_name="msCrypto", structural)] | ||
fn ms_crypto(me: &Self_) -> JsValue; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting nit here:
type Self_;
#[wasm_bindgen(method, getter, structural)]
fn crypto(me: &Self_) -> JsValue;
#[wasm_bindgen(method, getter, js_name = "msCrypto", structural)]
fn ms_crypto(me: &Self_) -> JsValue;
We should explain the use of msCrypto
above (when we use it)
src/wasm32_bindgen.rs
Outdated
@@ -64,9 +64,15 @@ fn getrandom_init() -> Result<RngSource, Error> { | |||
// `crypto.getRandomValues`, but if `crypto` isn't defined we assume | |||
// we're in an older web browser and the OS RNG isn't available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document the crypto vs mscrypto distinction here, so it's all in one place. Something like
// If `self` is defined then we're in a browser somehow (main window
// or web worker). We get `self.crypto` (called `msCrypto` on IE), so we
// can call `crypto.getRandomValues`. If `crypto` isn't defined, we
// assume we're in an older web browser and the OS RNG isn't available.
src/wasm32_bindgen.rs
Outdated
// If crypto is undefined, we can also check if msCrypto is defined, | ||
// in case it's Internet Explorer | ||
crypto = self_.ms_crypto(); | ||
|
||
if crypto.is_undefined() { | ||
return Err(BINDGEN_CRYPTO_UNDEF); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make this logic more straightforward using Rust's match guards:
let crypto: BrowserCrypto = match (self_.crypto(), self_.ms_crypto()) {
(crypto, _) if !crypto.is_undefined() => crypto.into(),
(_, crypto) if !crypto.is_undefined() => crypto.into(),
_ => return Err(BINDGEN_CRYPTO_UNDEF),
};
// Test if `crypto.getRandomValues` is undefined as well
if crypto.get_random_values_fn().is_undefined() {
return Err(BINDGEN_GRV_UNDEF);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that!
@zer0x64, also be sure to run |
Great, I'll address the changes tomorrow! For the CI, unfortunately, wasm-bindgen-test doesn't support headless IE, so the only way I see to test this in a CI is to use a framework like Selenium. It is certainly doable, but not as straightforward as testing Firefox/Chrome. Also, I don't think this can be done without a Windows build server. However, I can help integrate it if you decide to add this check to the CI! EDIT: Also forgot to mention that since IE does not support webassembly, we would need to use |
@zer0x64 would Could we just do:
and not need any additional code? I don't know if
This seems reasonable. We can always add more testing later once the tools are in a better state. Our coverage right now is fairly good (Node, Chrome, Firefox). |
I did some tests with the vendor_prefix and it simply doesn't seem to work in this context at all. I think it might be since the annotation is made for |
I've just pushed the fixes for your comments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just need #141 to be approved, then we can merge this.
EDIT: Actually, having this PR against the |
src/wasm32_bindgen.rs
Outdated
(crypto, _) if !crypto.is_undefined() => crypto.into(), | ||
(_, crypto) if !crypto.is_undefined() => crypto.into(), | ||
_ => return Err(BINDGEN_CRYPTO_UNDEF), | ||
}; | ||
|
||
// Test if `crypto.getRandomValues` is undefined as well | ||
let crypto: BrowserCrypto = crypto.into(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need this line (as we already convert the JsValue
to a BrowserCrypto
above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem!
Rebased and tested on my side, so when the CI pass it will be ready to merge! |
It is a pretty obscure use case, but I did stumble upon it:
When using wasm2js to compile WASM to Javascript for use in older browsers as recommended by the wasm-bindgen documentation, we found out that
rand
crate did not work on Internet Explorer 11 since thewindow.crypto
object is namedwindow.msCrypto
.This PR adds a check to use
msCrypto
ifcrypto
is not present.