daxpedda / web-time

Drop-in replacement for std::time for Wasm in browsers
Apache License 2.0
52 stars 8 forks source link

Casting web_time::SystemTime to std::time::SystemTime #17

Closed j-white closed 10 months ago

j-white commented 10 months ago

I ran into a case where I needed to cast an instance of web_time::SystemTime back to a std::time::SystemTime since another library's struct explicitly required that type.

I came up with this function as a bandaid:

fn to_std_systemtime(time: web_time::SystemTime) -> std::time::SystemTime {
    let ts_millis = time.duration_since(SystemTime::UNIX_EPOCH).unwrap().as_millis();
    let ts_secs = ts_millis / 1000;
    let ts_ns = (ts_millis % 1000) * 1_000_000 ;
    let dt = DateTime::<Utc>::from_timestamp(ts_secs as i64, ts_ns as u32).unwrap();
    return std::time::SystemTime::from(dt);
}

Is there a nicer way to do this? Would it make sense to include such a function in the lib?

Thanks!

notgull commented 10 months ago

On non-WASM, it should be the same type. On WASM without WASI, you shouldn't be able to instantiate std::time::SystemTime anyways.

daxpedda commented 10 months ago

I think the following would suffice:

fn to_std_systemtime(time: web_time::SystemTime) -> std::time::SystemTime {
    let duration = time.duration_since(web_time::SystemTime::UNIX_EPOCH).unwrap();
    std::time::SystemTime::UNIX_EPOCH + duration
}

The problem is that the library you are trying to be compatible with could be using std::time::SystemTime::now() internally, which would panic during runtime. So either way I would encourage you to check with the library if they could depend on web-time if it makes sense.

In any case, I think adding a conversion behind a trait doesn't hurt, though I will document that this might just be a footgun.

daxpedda commented 10 months ago

On that note, I will also transition to using Duration internally on SystemTime, right now it is using i64.

The original reason was that according to ECMA Date.now() can actually return negative numbers, but that isn't really practically possible in our use-case here. I'm sure there are some weird JS environments out there that might support something like this, but we are targeting Web and browsers, which shouldn't be supporting that.

j-white commented 10 months ago

Awesome - thanks for the simplified snippet and feedback!

daxpedda commented 10 months ago

I still plan on adding the conversion!