-
Notifications
You must be signed in to change notification settings - Fork 40
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
Re-enable web unit test #4444
Re-enable web unit test #4444
Conversation
f93a867
to
0774f3b
Compare
94ded32
to
691bcff
Compare
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.
LGTM (although the CI is complaining about wasm-pack
not being found)
66162db
to
063060d
Compare
15543dc
to
aa60c7b
Compare
c75a97d
to
0ee8013
Compare
@@ -109,6 +111,9 @@ mod not_wasm32 { | |||
} | |||
} | |||
|
|||
#[cfg(not(target_arch = "wasm32"))] | |||
pub use not_wasm32::{HTTPS_PROXY, HTTP_PROXY}; |
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.
the names to use in platform are native
and web
(don't use wasm32
given we specifically target web i.e. we consider we have access to the browser API)
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.
done
981a542
to
26b48c8
Compare
26b48c8
to
d8b22fc
Compare
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.
👍
@@ -1,80 +1,160 @@ | |||
#! /usr/bin/env node | |||
|
|||
const { argv, exit, platform } = require('node:process'); | |||
const { spawnSync } = require('node:child_process'); | |||
const { spawnSync, spawn } = require('node:child_process'); |
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.
spawn
doesn't seems used
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 it isn't, all good now :)
Remove `wasm-pack` from `devDependencies` since it don't work. Instead install it from the wasm-pack repo. Other Changes ------------- - Correct unused warning when compiling `platform_http_proxy` on `wasm32`. - In `bindings/web/scripts/build.js`: - Provide `process.env` to sub command. - Now the script will directly invoque `wasm-pack`. - Ensure rust target `wasm32-unknown-unknown` is installed. - Ensure a link exist between `ci-rust` & `debug`. - Cache rust artifact when build the web bindings. Closes #4447
d8b22fc
to
d27847a
Compare
Remove
wasm-pack
fromdevDependencies
since it don't work.Instead install it from the wasm-pack repo.
Other Changes
platform_http_proxy
onwasm32
.bindings/web/scripts/build.js
:process.env
to sub command.wasm-pack
.wasm32-unknown-unknown
is installed.ci
build type for rust web sincewasm-pack
don't support custom profile.Closes #4447