Skip to content

Fix dates being initialized with client timezone when using timestamps #288

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

Merged
merged 13 commits into from
Apr 6, 2023

Conversation

AppelBoomHD
Copy link
Contributor

@AppelBoomHD AppelBoomHD commented Mar 19, 2023

Timestamps in mysql and postgressql are by default save as UTC timestamps.
Javascripts Date initializer assumes that dates are written in the client's timezone, when no explicit timezone is given in the initializer. This PR manually adds the UTC timezone to the initializer string.

Another way to this is (https://stackoverflow.com/a/3075893/13799537):

// Split timestamp into [ Y, M, D, h, m, s ]
var t = "2010-06-09 13:12:01".split(/[- :]/);

// Apply each element to the Date function
var d = new Date(Date.UTC(t[0], t[1]-1, t[2], t[3], t[4], t[5]));
return new Date(d);

In TypeScript this gives a couple type-errors, which is why I chose the more simple one-liner solution.

@dankochetov
Copy link
Contributor

Honestly, I'm not sure if there are hidden implications of adding this. Could you add an integration test for this behavior?

@AppelBoomHD
Copy link
Contributor Author

I'm not very familiar with unit testing, but I think I got it working correctly.
Added a test for mysql and postgres databases to test whether the timestamp received from the database is converted to the correct timezone. The test for mysql fails before my change and passes after applying the change.
I realized postgres automatically converts the timestamp to the correct timezone, so I reverted the change related to postgres timestamps.

test.serial('timestamp timezone', async (t) => {
const { db } = t.context;

await db.insert(usersTable).values({ name: 'John' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test inserting a Date with a non-default timezone specified? I'm curious if it'll change the date string format returned from the driver on selection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I added the test as you specified for both mysql and postgres timestamps. Upon running the tests I found out that timestamps are by default inserted respecting the client's timezone as well, instead of in UTC format. I pushed a fix for this behavior, which passed all the tests.

@AppelBoomHD AppelBoomHD requested a review from dankochetov April 3, 2023 10:03
Copy link
Contributor

@dankochetov dankochetov left a comment

Choose a reason for hiding this comment

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

Seems like you have a code formatter (Prettier, probably) enabled on save, which changed the formatting quite a bit. We use Dprint for this repo, so please re-format the files with it (it might not rollback some formatting you already did, so you'll have to change it back manually).

@AppelBoomHD AppelBoomHD requested a review from dankochetov April 3, 2023 11:51
@dankochetov
Copy link
Contributor

@AppelBoomHD please resolve the merge conflicts

@AppelBoomHD AppelBoomHD force-pushed the main branch 2 times, most recently from a3e11c8 to 7293f88 Compare April 6, 2023 10:51
@dankochetov dankochetov merged commit 09a5a3e into drizzle-team:main Apr 6, 2023
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