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.

Sign up for GitHub

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

Jump to bottom

fix: ISO date parsing #10011

Closed
wants to merge 2 commits into from
Closed

fix: ISO date parsing #10011

wants to merge 2 commits into from

Conversation

felix-gohla
Copy link
Contributor

@felix-gohla felix-gohla commented May 3, 2023 •
edited

Description of change

Fix date parsing ignoring time zones.
In the code there's a explanation with link to the ECMA-script specification on why that happens.
The error was introduced when removing date-fns from typeorm in #9634.

Discussion

However: I wonder if it is even intended to deviate from the standard (even if the distinction between date and date-times seems a little bit... odd). Currently, the parse function always assumes local time unless there's a timezone given. This could be confusing.

During testing I also noticed that there are some tests that relied on another date format.
Maybe it would be wise to decide which date standard to go for (RFC3339 or ISO8601, see this website for comparison).
Furthermore, not sure if it makes sense to handle this by code in typeorm itself.
There are other people who really already invested time into figuring dates out in JS.

Fixes #9941

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run format to apply prettier formatting
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@felix-gohla felix-gohla changed the title Fix/9941 date parsing fix: ISO date parsing May 3, 2023
@felix-gohla felix-gohla mentioned this pull request May 3, 2023
Closed
henrinormak
henrinormak approved these changes May 3, 2023
View reviewed changes
src/util/DateUtils.ts Outdated
Comment on lines 295 to 301
console.log(
"isIsoDataTime",
isDateTime,
isoDateString,
parsed,
parsed.toISOString(),
)
Copy link

Choose a reason for hiding this comment

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

Suggested change
console.log(
"isIsoDataTime",
isDateTime,
isoDateString,
parsed,
parsed.toISOString(),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, how did I not catch that? ^^

src/util/DateUtils.ts Outdated

const isDateTime = isoDateString.includes("T")
if (!isDateTime) {
// This lets the date parser now that the date is to be treat as local time.
Copy link

Choose a reason for hiding this comment

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

Suggested change
// This lets the date parser now that the date is to be treat as local time.
// This lets the date parser know that the date is to be treat as local time.

@henrinormak
Copy link

Not a maintainer of the repo so can't help with getting this merged, but really looking forward to the fix for the issue this is targetting (as it is affecting me as well).

@pleerock
Copy link
Member

pleerock commented May 3, 2023

thanks @felix-gohla for taking a look on this one. Tests are failing however...

@felix-gohla
Copy link
Contributor Author

I appreciate your response. I have refrained from fixing them until now because there are some discussion parts (see PR description). 😊
As a maintainer, maybe you could give your opinion.

If you say that typeorm should handle all the cases itself (without external library), then I can continue and fix the tests.
However, there are more edge cases on date handling, IMHO. This could make the code more complex (which you didn't want, see #9634 (comment)).

@felix-gohla
test: add test for ISO date parsing issue typeorm#9941
d7c3674 
Add a test for date handling functions that seem to ignore the timezone offsets on insert.

Related to typeorm#9941
Introduced in typeorm#9634
@felix-gohla
fix: parse ISO times taking local time into account
81a8d8a 
Add handling for the different timezones during parsing ISO dates as there's
a difference in date parsing and date-times parsing in the ECMA-script specs.
Fix the bit where even dates with given timezones were parsed as local time.

Related To typeorm#9941
@pleerock
Copy link
Member

pleerock commented May 9, 2023

Closed by 54f4f89

@pleerock pleerock closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrinormak henrinormak henrinormak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ISO DateTime parsing returning wrong Date value
3 participants
@felix-gohla @henrinormak @pleerock