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

V8 patch for v8::Date::ToUTCString and v8::Date::Parse #1724

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 24, 2024

No description provided.

Patches v8 to expose new date methods to avoid having to grab
the methods from the prototype object.
@jasnell jasnell requested review from a team as code owners February 24, 2024 16:02
@jasnell jasnell requested review from ObsidianMinor and garrettgu10 and removed request for a team February 24, 2024 16:02
@kentonv
Copy link
Member

kentonv commented Feb 24, 2024

I'm not a big fan of this, especially the part where the implementation is copied. We won't get bugfixes from the original. Anyway, we should try to keep a light touch with V8 patches if at all possible.

I don't think it's so bad to read the properties off the JS objects, can we just leave it that way?

@jasnell
Copy link
Member Author

jasnell commented Feb 26, 2024

I will be contributing these changes directly to v8. Once that lands there, we can either float that version of the patch until we pick it up in the regular v8 update or keep this PR pending until the update.v

https://groups.google.com/g/v8-dev/c/Ezua_w2NH_M

@jasnell jasnell marked this pull request as draft February 26, 2024 15:15
@kentonv
Copy link
Member

kentonv commented Feb 26, 2024

I will be contributing these changes directly to v8.

That sounds great!

@jasnell
Copy link
Member Author

jasnell commented Apr 10, 2024

@jasnell jasnell added the v8 label Apr 10, 2024
@jasnell
Copy link
Member Author

jasnell commented Apr 22, 2024

Happy days... the v8 change has been accepted. Will just be waiting on us to receive the v8 update that includes the new APIs and this PR can be updated to drop the patch and move forward

@jasnell
Copy link
Member Author

jasnell commented Apr 30, 2024

Looks like it'll be v8 12.6 before the upstream change will land

@jasnell
Copy link
Member Author

jasnell commented May 17, 2024

Superseded by #2137

@jasnell jasnell closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants