-
Notifications
You must be signed in to change notification settings - Fork 754
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
Use web-time
on Wasm
#2758
base: master
Are you sure you want to change the base?
Use web-time
on Wasm
#2758
Conversation
Rebased on |
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.
Overall, I think this change seems good to me! I have some questions, but once those are addressed, I think we can probably move forwards with this. In particular:
- I wasn't sure about the feature flagging, which I left some inline comments on.
- Would it be possible to add some kind of tests for the webassembly support for
fmt::Layer
? In other crates, we are able to run some tests for wasm targets usingwasm-bindgen-test
; is that possible here?
@@ -32,6 +32,7 @@ fmt = ["registry", "std"] | |||
ansi = ["fmt", "nu-ansi-term"] | |||
registry = ["sharded-slab", "thread_local", "std"] | |||
json = ["tracing-serde", "serde", "serde_json"] | |||
wasm-bindgen = ["web-time"] |
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.
is this necessary as a feature flag, or can we just make the dependency always be used if fmt
is enabled and we're on a WASM target? Are there reasons that a user might want to compile for a WASM target without WASI, but still disable this feature?
Also, this feature flag should probably not introduce the additional dependency unless the fmt
feature is also enabled.
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.
is this necessary as a feature flag, or can we just make the dependency always be used if
fmt
is enabled and we're on a WASM target?
We can.
Are there reasons that a user might want to compile for a WASM target without WASI, but still disable this feature?
Yes, they might be using the wasm32-unknown-unknown
target but not targeting the Web. Spans won't be working for them but they might still be using tracing-subscriber
(as were Wasm on Web users until now). Though this is probably niche.
Also, this feature flag should probably not introduce the additional dependency unless the
fmt
feature is also enabled.
I'm not aware of a way to do this. But if it's decided to enable it by default anyway we could just add it to the fmt
crate feature.
I know that there are no-std Wasm users out there that can't rely on wasm-bindgen
, but I don't know if they are using tracing-subscriber
or not. It's also possible that a new Web backend gets popular, besides wasm-bindgen
, in which case it would be useful to have separate crate features, though I'm not aware of any current development in that area.
Let me know how you want to proceed.
//! - `wasm-bindgen`: Uses the [`web-time`] crate to allow `Uptime` and `SystemTime` to | ||
//! be used in browsers. |
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.
it seems a bit strange to me that the feature flag is wasm-bindgen
rather than the actual name of the optional dependency (web-time
)...is this a standard practice for feature flag naming in the wasm ecosystem?
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.
On the other hand, I suppose if we want to add additional WASM support, such as using Console.log
with more structured values when running in the browser, we may want to have one big feature flag for that.
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.
it seems a bit strange to me that the feature flag is
wasm-bindgen
rather than the actual name of the optional dependency (web-time
)...is this a standard practice for feature flag naming in the wasm ecosystem?
This was done in the past when stdweb
was still a thing, because there were two different Wasm Web backends: wasm-bindgen
and stdweb
.
But yes, the name refers to the Wasm Web backend.
It is possible, I briefly looked into it, but it didn't make much sense to me because I found some tests that test tracing/tracing-subscriber/src/fmt/mod.rs Lines 1316 to 1329 in f93cfa0
tracing/tracing-subscriber/src/fmt/fmt_subscriber.rs Lines 1235 to 1251 in f93cfa0
I can look into hooking those up? |
Rebased on master, CI currently fails with the same issue as on master. |
Rebased on master after #2814, so now CI is green. |
Updated to |
And another ping. :-) |
Bump |
Motivation
Currently
Uptime
,SystemTime
and spans panic on browsers, this is becausestd::time
is not supported in browsers.Even external layers, like
tracing-web
, are currently not able to circumvent the issue with spans.Solution
The solution is very straightforward: not using
std::time
when on Web. This PR usesweb-time
instead.This will only be enabled when on Wasm and when enabling the
wasm-bindgen
crate feature, otherwise no dependencies are added.Alternatively I could import some of the necessary code from
web-time
if this is more desirable.I originally wanted to fix some of these issues in #2533, but it turned out that
instant
wasn't as accurate a drop-in replacement as I expected, in addition to the bugs and missing features/improvements, which was the original motivation to makeweb-time
.Partly addresses #642.
Fixes #2720.