Skip to content

Conversation

@apoco
Copy link

@apoco apoco commented Oct 31, 2014

Dates are stored in SQLite as Unix timestamps. Dates are represented in JavaScript as Unix timestamps. Thus, there need not be any sort of conversions. Client-side timezones are irrelevant; they should only affect the client-side display of dates. The existing code is incorrect because it's storing timezone-offset dates as UTC dates (with the "Z" suffix).

This change simplifies the type conversion; a Date is stored as an ISO-8601 date string, which is understood by both JavaScript's Date constructor and SQLite.

Dates are stored in SQLite as Unix timestamps. Dates are
represented in JavaScript as Unix timestamps. Thus, there need
not be any sort of conversions. Client-side timezones are
irrelevant; they should only affect the client-side display of
dates. The existing code is incorrect because it's storing
timezone-offset dates _as UTC dates_ (with the "Z" suffix).

This change simplifies the type conversion; a Date is stored
as an ISO-8601 date string, which is understood by both
JavaScript's `Date` constructor and SQLite.
@dxg
Copy link
Collaborator

dxg commented Nov 13, 2014

Looks good, I'll just have to run this against ORM

@apoco
Copy link
Author

apoco commented Nov 15, 2014

Great. I have some changes locally that work in conjunction with this, but I hesitated creating a PR since it won't pass without this package's updates. Would it be helpful for me to put a PR together, or is this pretty straightforward?

Another option that occurred to me is maybe having a different data type for this, like isodate; that way, if there is anyone that is using the current behavior would not be affected by the change. What are your thoughts? I'm not in a big rush because I went with just using strings for now.

@dxg
Copy link
Collaborator

dxg commented Nov 16, 2014

I've continued discussion in dresende/node-orm2#568
Feel free to open the pull request and write a comment about why the tests fail inside.

assert.equal(
dialect.escapeVal(new Date(d.getTime()), '-0400'),
"'2013-09-04T15:15:11.133Z'"
"'2013-09-04T19:15:11.133Z'"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect this test will fail on slower machines.
It should probably use http://sinonjs.org/releases/v4.4.2/fake-timers/ instead.

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