Skip to content

Conversation

@flakey5
Copy link
Member

@flakey5 flakey5 commented Apr 29, 2025

No description provided.

@flakey5 flakey5 requested a review from a team as a code owner April 29, 2025 17:20
@flakey5 flakey5 linked an issue Apr 30, 2025 that may be closed by this pull request
@@ -0,0 +1,295 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

This is grabbed straight from undici, do we really need to have a copy of this in our code? Is there a 3rd party library we can use? I really want to avoid us from maintaining such kind of code.

Copy link
Member

@MattIPv4 MattIPv4 May 21, 2025

Choose a reason for hiding this comment

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

@nodejs/undici I hope you don't mind the ping, but is this date parsing logic something y'all would consider exposing?

It looks like a couple of utils are currently exposed: https://github.com/nodejs/undici/blob/674ae24d6e61a3ef62b4b3e8677fcae52c0fa068/index.js#L60-L63

So, would it be viable to add https://github.com/nodejs/undici/blob/674ae24d6e61a3ef62b4b3e8677fcae52c0fa068/lib/util/date.js to that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is grabbed straight from undici, do we really need to have a copy of this in our code? Is there a 3rd party library we can use? I really want to avoid us from maintaining such kind of code.

Why not? We have parsing logic already for other things such as the range header

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM!

@flakey5
Copy link
Member Author

flakey5 commented May 22, 2025

I'm gonna merge & deploy this now just to stop us from getting spammed with more alerts, but we can continue talking about if we want to use a package for this here or in an issue or on Slack if y'all want

@flakey5 flakey5 merged commit 0e2154e into main May 22, 2025
6 checks passed
@flakey5 flakey5 deleted the flakey5/399 branch May 22, 2025 03:07
@MattIPv4
Copy link
Member

I think best thing would be an issue or PR in undici to expose this util?

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.

TypeError: This API doesn't support dates before 1687.

4 participants