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

NaN Date created from what seems to be a valid Date string #766

Closed
olliejm opened this issue Jul 23, 2020 · 6 comments · Fixed by #1434
Closed

NaN Date created from what seems to be a valid Date string #766

olliejm opened this issue Jul 23, 2020 · 6 comments · Fixed by #1434

Comments

@olliejm
Copy link

olliejm commented Jul 23, 2020

As a followup to the Date issue I mentioned yesterday, I noticed that my Date should not have been getting constructed with a NaN argument in the first place.

The value passed to the Date constructor is the string Thu, 30 Jan 2020 08:00:00 PST.

I have inspected, and that is indeed the value of var date at DateConstructor L97.

However all the checks fail and the statement return JsNumber.DoubleNaN; is executed.

Is that expected? Constructing a Date from this string succeeds in the browser console:

new Date("Thu, 30 Jan 2020 08:00:00 PST");

Outputs:

> Date Thu Jan 30 2020 16:00:00 GMT+0000 (Coordinated Universal Time)

I can reproduce it with this unit test:

[Fact]
public void CanParseLocaleString()
{
    var result = _engine.Execute("new Date(\"Thu, 30 Jan 2020 08:00:00 PST\");").GetCompletionValue();

    Assert.True(result.IsDate() && !double.IsNaN(result.AsDate().PrimitiveValue));
}
@lahma
Copy link
Collaborator

lahma commented Jul 23, 2020

The thing is that per specification:

The function first attempts to parse the String according to the format described in Date Time String Format (20.4.1.15), including expanded years. If the String does not conform to that format the function may fall back to any implementation-specific heuristics or implementation-specific date formats. Strings that are unrecognizable or contain out-of-bounds format element values shall cause Date.parse to return NaN.

The standard format does not contain this example here and .NET cannot parse PST directly as it needs mapping to correct time zone data (offset). So this case is not covered by the spec and is not strictly required (it's the "implementation-specific heuristics or implementation-specific date formats" part).

@sebastienros
Copy link
Owner

Should we allow for an option that contains extra formats to configure?

@lahma
Copy link
Collaborator

lahma commented Jul 23, 2020

I did some (non-definitive) googling and what I understood this would require more than string pattern as 'PST' (or anything non-UTC) would require custom parsing or string replacement. Not sure how other libraries handle this.

@IntranetFactory
Copy link

It's quite surprising that there is such a gaping hole for Date parsing formats in the spec. V8 seems to support quite a lot of formats: https://github.com/v8/v8/blob/master/test/mjsunit/date-parse.js#L61

For our use cases we needed at least support for RFC 822 date format (https://www.w3.org/Protocols/rfc822/#z28). For now we integrated MimeKit.DateUtils (https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Utils/DateUtils.cs) as an additional parser to cover RFC 822.

Do you have any plans to support more date formats to close the gap to V8 and/or allowing to plugin your own date parser?

@lahma
Copy link
Collaborator

lahma commented Jul 29, 2020

Supporting RFC 822 in Jint sounds logical. The solution linked is MIT licensed (can be used with attribution) and quick googling finds alternatives too.

@sebastienros
Copy link
Owner

Sounds good to parse this specifically then. Either by copying the code from MimeKit + License, or creating our own. We don't need to use DateTimeOffset or anything I assume since we already have all the primitives to build js dates, and only need to add custom timezone mappings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants