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

Convert saved non-UTC dates to UTC #10967

Merged
merged 3 commits into from
May 8, 2024
Merged

Convert saved non-UTC dates to UTC #10967

merged 3 commits into from
May 8, 2024

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented May 7, 2024

Changes

Testing

  • Tried really hard to write a test, but because the database is running locally the date's already in the "correct" time zone. Would love a suggestion here if anyone can think of one.

Docs

N/A, bug fix

Copy link

changeset-bot bot commented May 7, 2024

🦋 Changeset detected

Latest commit: ae2e831

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

LGTM!

if(!isISODateString(value)) {
// values saved using CURRENT_TIMESTAMP are not valid ISO strings
// but *are* in UTC, so append the UTC zone.
value += '.000Z';
Copy link
Member

Choose a reason for hiding this comment

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

Did a quick tests and += 'Z' is actually all it takes — the milliseconds are irrelevant fwiw.

Comment on lines 24 to 25
const d = new Date(str);
return d instanceof Date && !isNaN(d.getTime()) && d.toISOString() === str; // valid date
Copy link
Member

Choose a reason for hiding this comment

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

Do we even need to be this cautious? What are the chances something that matches the ISO pattern is invalid if we’re adding it to the DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, is there a downside to doing this though (I just grabbed from stackoverflow)

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's copied code we now own, I'd just leave a comment to the source

Copy link
Member

Choose a reason for hiding this comment

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

is there a downside to doing this though

I guess only extra work for each Date parsed? If you fetched a lot of data, you’re now constructing the Date twice for each one. I don’t really mind leaving it in though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed extra bits, it's just the regex now.

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

That was UTeasy

Comment on lines 24 to 25
const d = new Date(str);
return d instanceof Date && !isNaN(d.getTime()) && d.toISOString() === str; // valid date
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's copied code we now own, I'd just leave a comment to the source

@matthewp matthewp merged commit a134318 into main May 8, 2024
13 checks passed
@matthewp matthewp deleted the fix-timezone branch May 8, 2024 12:27
@astrobot-houston astrobot-houston mentioned this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants