Skip to content

Replace unclear fromStandardDate() #1290

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

Open
wants to merge 7 commits into
base: 6.x
Choose a base branch
from
Open

Replace unclear fromStandardDate() #1290

wants to merge 7 commits into from

Conversation

MaxAake
Copy link
Contributor

@MaxAake MaxAake commented May 27, 2025

The Neo4j.Date functions fromStandardDate() and toStandardDate() are not inverse functions of each other, as fromStandardDate() takes the date that the StandardDate would correspond to in local time, and toStandardDate() creates a Date set at Midnight UTC.

This PR deprecates the old fromStandardDate() and replaces it with a drop-in replacement fromStandardDateLocal() which functions the same and fromStandardDateUTC() which takes the date as interpreted in UTC.

Copy link
Member

@robsdedude robsdedude left a comment

Choose a reason for hiding this comment

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

🔥 ⌚ 🙅

🙏 ☮️

Copy link
Member

Choose a reason for hiding this comment

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

My brain was close to melting while trying to understand this PR 🫠

I think example code would go a long way. Something like this (maybe split the examples and spread them across the functions' docs)

// will do what it says on the tin
Neo4jDate.fromStandardDateLocal(new Date(2020, 01, 01))
Neo4jDate.fromStandardDateUTC(new Date(Date.UTC(2020, 01, 01)))

// Depending on the host's locale, might end up with a different calendar date
// than 2020-01-01
Neo4jDate.fromStandardDateLocal(new Date(Date.UTC(2020, 01, 01)))
Neo4jDate.fromStandardDateUTC(new Date(2020, 01, 01))

Copy link
Member

Choose a reason for hiding this comment

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

Optional: I think it might be very much worthwhile to run these tests for different time zones/locales. In particular the ones that are far off of UTC (and maybe also UTC itself).

https://stackoverflow.com/questions/8083410/how-can-i-set-the-default-timezone-in-node-js

Comment on lines +398 to +399
* fromStandardDateLocal(new Date("2010-10-10T00:00:00")) // This may cause issues as JS Dates are created at local time by default
* fromStandardDateLocal(new Date("2010-10-10T00:00:00Z")) // Will create a date at 2010-10-10 as this date is created at UTC with the trailing "Z"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* fromStandardDateLocal(new Date("2010-10-10T00:00:00")) // This may cause issues as JS Dates are created at local time by default
* fromStandardDateLocal(new Date("2010-10-10T00:00:00Z")) // Will create a date at 2010-10-10 as this date is created at UTC with the trailing "Z"
* fromStandardDateUTC(new Date("2010-10-10T00:00:00")) // This may cause issues as JS Dates are created at local time by default
* fromStandardDateUTC(new Date("2010-10-10T00:00:00Z")) // Will create a date at 2010-10-10 as this date is created at UTC with the trailing "Z"

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.

2 participants