only include the chrono wasmbind feature in wasm arch's #262
Conversation
| serde_path_to_error = "0.1.2" | ||
|
|
||
| [target.'cfg(target_arch = "wasm32")'.dependencies] | ||
| getrandom = { version = "0.2", features = ["js"] } | ||
|
|
||
| [target.'cfg(target_arch = "wasm32")'.dependencies.chrono] | ||
| features = ["wasmbind"] |
There was a problem hiding this comment.
this triggers a new warning:
dependency (chrono) specified without providing a local path, Git repository, version, or workspace dependency to use. This will be considered an error in future versions
I think the simplest fix may be to add the entire current chrono = ... line above getrandom = ... (line 65).
I'm also a bit worried that there's effectively no test coverage for this, since the WASM chrono issue used to cause a panic at runtime. Disabling the chrono wasmbind feature doesn't break compilation, and unit tests aren't runnable on WASM targets to check for runtime behavior. The wasm-bindgen-test crate exists though, and maybe we can add a simple test to make sure this crate's chrono dependency works as expected on WASM targets.
There was a problem hiding this comment.
Arrh, thanks, I will try to fix it. I wll try to add the wasm-bindgen-test, do you have an idea for a test case that will test the chrono dependency? Not sure exactly caused the panic before
There was a problem hiding this comment.
Thanks!
do you have an idea for a test case that will test the chrono dependency?
I think the issue originally popped up in the downstream openidconnect crate, which relies on timestamps a lot more for things like checking whether ID tokens are expired. My guess is that just calling Utc::now() in a WASM build should suffice to test whether or not chrono supports WASM, since it'll need to use a WASM interface for getting the current time. Hopefully a wasm-bindgen-test test that calls Utc::now() will fail without chrono's wasmbind feature enabled and pass with the feature enabled.
There was a problem hiding this comment.
I can add it, but isn't this just testing another crate, and perhaps should be added upstream?
There was a problem hiding this comment.
This was the initial issue I encountered with wasm: ramosbugs/openidconnect-rs#127 (comment) so yeah a test on a field that uses Utc::now() should be enough, as for the test, it could be just instanciating a structure that uses Utc::now(), @ramosbugs could give an example I think since I haven't touched this part of the code in a while.
There was a problem hiding this comment.
I can add it, but isn't this just testing another crate, and perhaps should be added upstream?
I'd say this is testing oauth2's usage of another crate. Specifically, it's testing that we imported chrono correctly with the features required by this crate.
I would prefer if chrono enabled wasmbind automatically for WASM builds since I'm not sure what good that crate is in WASM builds otherwise (I guess it can parse dates and do math without needing to know the current system time?). Or, it should at least remove functions like Utc::now in WASM builds without the wasmbind feature. But, given that it doesn't, it's worth adding a test to prevent future regressions since we've already had this bug once before. I'll add some discussion in chronotope/chrono#1301, but I wouldn't expect it to change soon.
There was a problem hiding this comment.
I added some suggestions to the chrono issue. After looking at chrono's Cargo.toml, @SorenHolstHansen, I'm wondering whether wasm-bindgen is actually being compiled at all for non WASM targets:
https://github.com/chronotope/chrono/blob/7f57dde63ce66218f624af7f561622e7906c3d06/Cargo.toml#L47-L49
Just because it's included in your Cargo.lock (which I believe is agnostic to build targets since the same Cargo.lock can compile for multiple targets) doesn't mean it's actually being compiled. Do we need to make this change at all? What is it actually accomplishing?
There was a problem hiding this comment.
I think you are actually right. It is included in the Cargo.lock file, but doesn't seem to get compiled at all when looking at it.
Sorry for the confusion then, and thanks for the help :)
I looked at the Cargo.lock of my api, and found that wasm-bindgen was included because of the chrono dependency of this crate, so thought I would remove it from default