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

feat: impl Date API #639

Merged
merged 12 commits into from
Jan 13, 2021
Merged

feat: impl Date API #639

merged 12 commits into from
Jan 13, 2021

Conversation

amilajack
Copy link
Member

@amilajack amilajack commented Nov 21, 2020

Implements neon-bindings/rfcs#18

  • impl JsDate::new
  • impl JsDate::new_lossy
  • impl JsDate::try_new
  • impl JsDate::is_date
  • impl JsDate::is_valid
  • Write tests

@amilajack amilajack marked this pull request as draft November 21, 2020 18:20
@amilajack amilajack force-pushed the feat/date-api branch 3 times, most recently from 0999e56 to 342d740 Compare November 21, 2020 18:56
@amilajack amilajack force-pushed the feat/date-api branch 2 times, most recently from a716bcc to f538442 Compare November 21, 2020 21:11
@amilajack amilajack marked this pull request as ready for review November 21, 2020 21:20
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few minor comments.

crates/neon-runtime/src/napi/date.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/date.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/date.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/date.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
@amilajack amilajack force-pushed the feat/date-api branch 2 times, most recently from 853a345 to aef0f86 Compare November 22, 2020 20:23
@arthrarnld
Copy link

Hey peeps. Any news on this one? Having it would be really helpful in our team. Thanks!

@kjvalencik
Copy link
Member

kjvalencik commented Dec 7, 2020

@arthrarnld It's a work in progress. We're still discussing the best way to handle an out of range time. Let us know if you have any thoughts! If you need this feature now, it can be implemented outside of Neon.

fn global_date<'a, C: Context<'a>>(cx: &mut C) -> JsResult<'a, JsFunction> {
    cx.global()
        .get(cx, "Date")?
        .downcast::<JsFunction>()
        .or_throw(cx)
}

fn create_date<'a, C: Context<'a>>(
    cx: &mut C,
    n: f64,
) -> JsResult<'a, JsObject> {
    let date = global_date(cx)?;
    let args = vec![cx.number(n)];

    date.construct(cx, args)
}

// Doesn't really validate that it's a date, just that it has a `now()` function
// that returns a `number`. Could validate with `isPrototypeOf`, but probably not worth it.
fn date_value<'a, C: Context<'a>>(
    cx: &mut C,
    o: Handle<JsObject>,
) -> NeonResult<f64> {
    let now = o.get(cx, "now")?.downcast::<JsFunction>().or_throw(cx)?;
    let args = Vec::<Handle<JsValue>>::with_capacity(0);
    let res = now.call(cx, o, args)?.downcast::<JsNumber>().or_throw(cx)?;

    Ok(res.value())
}

@amilajack amilajack force-pushed the feat/date-api branch 4 times, most recently from 1e5ae76 to 072d5bf Compare January 1, 2021 05:35
test/napi/src/js/date.rs Outdated Show resolved Hide resolved
@amilajack amilajack requested a review from dherman January 1, 2021 08:13
src/types/mod.rs Outdated Show resolved Hide resolved
@amilajack amilajack force-pushed the feat/date-api branch 3 times, most recently from 5818563 to 352737e Compare January 7, 2021 02:19
@amilajack amilajack force-pushed the feat/date-api branch 2 times, most recently from e73c716 to 24ab88e Compare January 7, 2021 04:09
crates/neon-runtime/src/napi/bindings/functions.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/bindings/functions.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/bindings/functions.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/date.rs Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
src/types/mod.rs Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/date.rs Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/handle/mod.rs Show resolved Hide resolved
src/types/date.rs Outdated Show resolved Hide resolved
src/types/date.rs Outdated Show resolved Hide resolved
src/types/date.rs Show resolved Hide resolved
src/types/date.rs Show resolved Hide resolved
src/types/date.rs Outdated Show resolved Hide resolved
src/types/date.rs Show resolved Hide resolved
src/types/date.rs Show resolved Hide resolved
@amilajack amilajack requested a review from kjvalencik January 12, 2021 18:15
src/types/date.rs Outdated Show resolved Hide resolved
Copy link
Member

@kjvalencik kjvalencik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amilajack This looks great! Before merging, let's decide whether we want this feature flagged or not prior to merging the RFC.

src/context/mod.rs Outdated Show resolved Hide resolved
src/types/date.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants