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

RFC: Date API #18

Merged
merged 5 commits into from
Jul 2, 2021
Merged

RFC: Date API #18

merged 5 commits into from
Jul 2, 2021

Conversation

dherman
Copy link
Contributor

@dherman dherman commented Mar 22, 2018

A simple API for exposing JS Date objects to Rust.

Rendered

text/0000-date-api.md Outdated Show resolved Hide resolved
text/0000-date-api.md Outdated Show resolved Hide resolved
@goto-bus-stop goto-bus-stop changed the base branch from master to main October 8, 2020 13:45
…t for `value()` for compatibility with N-API.

Add an `is_valid()` method for checking if a `Date` value is in the legal range.
@amilajack
Copy link
Member

@kjvalencik @dherman should this be merged if ready?

@dherman
Copy link
Contributor Author

dherman commented Nov 24, 2020

We still have a few open design questions to settle before it's quite ready to merge. Once we do we'll put it into "final comment period" for a week before merging.

pub const MIN_VALID_VALUE: f64 = -8640000000000000;
pub const MAX_VALID_VALUE: f64 = 8640000000000000;

pub fn new<'a, C: Context<'a>, V: Into<f64>>(_: &mut S, value: V) -> JsResult<JsDate>;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be a JsResult since the Date API shouldn't throw unless we are already in a throwing state.

I do think it should return an Error if the value is out of bounds. I don't think it's safe to depend on being able to get the original value back out since that's not part of the contract/spec.

impl JsDate {
    /// Produces a _valid_ `Date` or a `DateError`
    pub fn new<'a, C: Context<'a>, V: Into<f64>>(_: &mut S, value: V) -> Result<Handle<'a, JsDate>, DateError>;

    /// Produces a `Date` which _could_ be invalid
    pub fn new_unchecked<'a, C: Context<'a>, V: Into<f64>>(_: &mut S, value: V) -> Handle<'a, JsDate>;
}

enum DateError {
    OutOfRangeLow,
    OutOfRangeHigh,
}

impl Error for DateError {}

impl<'a> JsResultExt<'a, JsString> for Result<Handle<'a, JsDate>, DateError> {
    /// Creates a `RangeError` on error
    fn or_throw<'b, C: Context<'b>>(self, cx: &mut C) -> JsResult<'a, JsDate>;
}

pub const MAX_VALID_VALUE: f64 = 8640000000000000;

pub fn new<'a, C: Context<'a>, V: Into<f64>>(_: &mut S, value: V) -> JsResult<JsDate>;
pub fn value<'a, C: Context<'a>>(self, cx: &mut C) -> f64;
Copy link
Member

Choose a reason for hiding this comment

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

Note: We should include in the documentation that this could be NaN.

@amilajack
Copy link
Member

Should this be closed or merged? cc @kjvalencik @dherman

@kjvalencik kjvalencik added the final comment period last opportunity to discuss the proposed conclusion label Mar 11, 2021
@kjvalencik
Copy link
Member

@amilajack I've added the "final comment period" label. Let's give it a week and if no issues pop-up we can merge. 👍

@dherman dherman merged commit 35df434 into neon-bindings:main Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final comment period last opportunity to discuss the proposed conclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants