Skip to content
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

Add Date Conversion #75

Merged
merged 1 commit into from
Oct 24, 2021
Merged

Add Date Conversion #75

merged 1 commit into from
Oct 24, 2021

Conversation

gsquire
Copy link
Contributor

@gsquire gsquire commented Oct 23, 2021

This patch adds a From implementation for the chrono crate's Date type. I can also add a conversion for DateTime if need be.

If you feel like this is an acceptable PR, could you please add a hacktoberfest-accepted label?

Closes #8

This patch adds a From implementation for the chrono crate's Date type.
@nilslice
Copy link
Contributor

@gsquire - thank you! I will take a look and can definitely add the label. Much appreciated.

@nilslice nilslice changed the base branch from main to pr-75 October 24, 2021 21:20
@nilslice nilslice merged commit 0e2f547 into cloudflare:pr-75 Oct 24, 2021
@nilslice
Copy link
Contributor

@gsquire - I believe we actually need the inverse trait impl here, as JavaScript is the source of time. chrono expects to use the underlying system, and so it would seem that we actually can only support From<worker::Date> for some chrono type.

Maybe I am looking at this the wrong way though.. I have a test endpoint that uses the following:

router
    .get("/now", |_, _| {
        let now = chrono::Utc::now();
        let js_date: Date = now.date().into();
        Response::ok(format!("{}", js_date.to_string()))
    })

Which fails with:

panicked at 'time not implemented on this platform', library/std/src/sys/wasm/../unsupported/time.rs:39:9

@nilslice
Copy link
Contributor

ah, it appears there is a feature in chrono that must be enabled:

# Cargo.toml

[dependencies]
chrono = { version = "0.4", default-features = false, features = ["wasmbind"]}

I will add this to the worker-sandbox, and look into conditionally enabling this feature in the worker crate along with chrono itself so it is not imposed on all users.

@gsquire
Copy link
Contributor Author

gsquire commented Oct 25, 2021

Thanks for reviewing. Is it still the case that we need the inverse implementation? Or is that feature enablement sufficient for your use case? I'm happy to follow up with another fix if need be.

@nilslice
Copy link
Contributor

No, I misinterpreted the error message. We should be good to go with this as long as you agree the "/now" endpoint example is sufficient (in the worker-sandbox directory in the root). Thanks!

@gsquire gsquire deleted the date-conversion branch October 30, 2021 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl From/Into chrono::Date for worker::Date
2 participants