Skip to content

fix: modify to run with @pinojs/dateformat#614

Draft
Uzlopak wants to merge 4 commits into
pinojs:dateformatfrom
Uzlopak:patch-1
Draft

fix: modify to run with @pinojs/dateformat#614
Uzlopak wants to merge 4 commits into
pinojs:dateformatfrom
Uzlopak:patch-1

Conversation

@Uzlopak

@Uzlopak Uzlopak commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

@jsumners
I am not maintainer here, so I could not push into your branch.

@Uzlopak

Uzlopak commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

Basically SYS: is the same as GMT. I could just modify our dateformat to even avoid that slice(4) call.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good!

@jsumners jsumners left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there any technical reasons @pinojs/dateformat is not a drop-in replacement for dateformat?

@Uzlopak

Uzlopak commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

@jsumners

No, it was an accidental typo.

@jsumners

jsumners commented Aug 7, 2025

Copy link
Copy Markdown
Member

@jsumners

No, it was an accidental typo.

Sorry. I don't follow. I expected the only changes necessary would be replacing every occurrence of require('dateformat') with require('@pinojs/dateformat'). That does not seem to be the case. Are there technical reasons for this?

@Uzlopak

Uzlopak commented Aug 7, 2025

Copy link
Copy Markdown
Contributor Author

No, there are no technical reasons. I made a mistake, by making a capital F instead of a lowercase f. I will fix it. I am just currently commuting... I provide you with a fix in fee hours

@Uzlopak

Uzlopak commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

@jsumners
done

@Uzlopak

Uzlopak commented Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

@mcollina
@jsumners

Please review again.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm once our dateformat method hits 1.0.0

Comment thread lib/utils/format-time.js
@Uzlopak
Uzlopak marked this pull request as draft August 11, 2025 15:45
@Uzlopak

Uzlopak commented Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Changing this PR to draft. Will get back to it some day.

@jsumners

jsumners commented Oct 8, 2025

Copy link
Copy Markdown
Member

I'm going through our modules in alphabetical order and getting the updated for pino@10/dropping Node v18. I've made it down to pino-caller, so I'm still a ways away from getting to this one. But I'd like to see this get resolved before I get there. Do you have time to address this?

@Uzlopak

Uzlopak commented Oct 8, 2025

Copy link
Copy Markdown
Contributor Author

yes, i will take care of it today.

@Uzlopak

Uzlopak commented Oct 13, 2025

Copy link
Copy Markdown
Contributor Author

@jsumners

wdyt of this in combination with pinojs/dateformat#5 `

@Uzlopak

Uzlopak commented Oct 13, 2025

Copy link
Copy Markdown
Contributor Author

So.. modified this PR to use only the improved dateformat function and not the DateFormatter from pinojs/dateformat.

It is not the implementation for high performance, as for that you should instantiate the dateformatter. But this change should be already faster than the original dateformat function.

If we one day have interest in more performance, we then only need to use DateFormatter.

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.

3 participants