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

Reimplement date parser #611

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Reimplement date parser #611

merged 5 commits into from
Sep 26, 2024

Conversation

ghost
Copy link

@ghost ghost commented Sep 24, 2024

Refer to QuickJS to reimplement the date parser to support more formats.

Differences from QuickJS implementation for compatibility with the original parser of goja:

  • Use UTC in date-only forms of non-Date Time String Format.
  • Check the letters following valid time zone abbreviations to reject invalid time zone abbreviations like ZZZ.

Some differences from the original parser of goja:

  • Names of the days of the week are ignored.
  • Letters after the 3rd character of a long month name are ignored.
  • Any characters in brackets are ignored.

(These behaviors are consistent with Node.js v22. I haven't checked the source code of V8, I just modified some test cases in TestDateParse to test in Node.js to guess its behavior, after all, it's not in the ECMAScript specification.)

Overall, this new parser is more relaxed and supports more formats (not limited to those added in the test), but does less validity checks than the one in the Go standard library. Let me know if you don't want to miss some validity checks.

@dop251
Copy link
Owner

dop251 commented Sep 25, 2024

Hi.

Thanks for submitting this.

Rewriting the date parser has long been on my to-do list. There are a coupe of PRs (#546 and #391) which I deferred merging for this reason.

The code looks like a direct translation from C, with pointer arguments used as return values and pointers to string. It could benefit from making it a little bit more idiomatic. Having said that, I think it's a step forward so I could merge without it.

Could you please make sure that it covers the 2 PRs mentioned above (and the PRs themselves are consistent with Node)?

And also, there is a test case in the V8 code (https://chromium.googlesource.com/v8/v8/+/refs/heads/main/test/mjsunit/date-parse.js), it would be nice if it passed too.

@ghost
Copy link
Author

ghost commented Sep 26, 2024

The tests in the V8 source code now pass.

#546: All covered including 01-2006 and 01/2006 which are not supported by Node.js.

#391: Formats 2006-01-02 15, 20060102T150405, 2006-01-02 15+04:05, 2006-01-02 15-0405 and 2006-01-02 15Z are not supported by Node.js, nor by this parser. The results of parsing formats 02-01-2006 and 02.01.2006 15:04:05 may not be what the PR author wants, but they are consistent with Node.js.

I find that the date-only forms of non-Date Time String Format use local time in both Chrome and Firefox, which is inconsistent with the original goja parser, and I am inclined to change this behavior and implement it in 6ccc14a.

@dop251 dop251 merged commit 02193f3 into dop251:master Sep 26, 2024
4 checks passed
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.

1 participant