search

alpacahq / alpaca-ts

A TypeScript Node.js library for the https://alpaca.markets REST API and WebSocket streams.
ISC License
154 stars 42 forks source link

Parsing dates #15

Closed lbstr closed 3 years ago

lbstr commented 3 years ago

I'm splitting the topic of dates out of issue #9 since it warrants its own discussion. @117 and I have discussed this somewhat, but I'm not sure we drew a conclusion.

Background

Alpaca provides dates as ISO-8601 strings. During transport, this is the ideal format since it is always in UTC and won't change as it travels across machines.

For users to actually use these dates, they need to convert to Date or use a library like date-fns, moment, etc. Should we save them this step and automatically convert the ISO-8601 strings to Date?

Considerations

  1. Timezones are a common source of bugs. While the JavaScript Date internally stores the value as UTC, most methods present the date to you in the local machine's timezone. People tend to screw this up.

  2. Alpaca has a mixture of date-time and date data. For dates, like in the Calendar entity, you get YYYY-MM-DD. We would not want to convert that as a Date since it does not represent a point in time, but rather a whole day. So if we decide to convert ISO-8601 strings to Date, but leave the YYYY-MM-DD strings alone, is that confusing to the user?

lbstr commented 3 years ago

My opinion

I'm inclined to leave all dates alone and let the user decide how they want to work with them. I feel like they should see the dates in UTC so that they are forced to think about timezones. So ya... I opened an issue that I'm now arguing to close 😄

That being said, I would love to hear some other opinions.

117 commented 3 years ago

I think it makes sense to parse ISO-8601 strings to a Date object. Those libraries can accept a Date object easily if the user doesn't want to work with the native Date class.

I also argue that YYYY-MM-DD is a point in time, just not on the same scale as an ISO-8601 string. A Date object for this does provide utility to the user, and makes it easy retrieve the day/month/year by calling a method without having to parse the string themself.

lbstr commented 3 years ago

I guess the same argument we applied towards number applies to Date: there could be precision issues, so if you want the raw data, we'll provide it. I can get on board...

Regarding the YYYY-MM-DD strings... The bug I've seen several times is when someone converts that to a Date, it immediately becomes midnight on that day, in that machine's timezone. The problem is that midnight in New York is 9PM on the previous day in California. So simple date comparisons get screwed up because you've introduced time.

117 commented 3 years ago

Any further changes before 2.0 release? If not I'm going to go ahead and prepare the merge, double check things.

lbstr commented 3 years ago

@117 I’d love to tackle the dates but I’m afraid I won’t have any time in the near future. If someone wants to go for it, it would be the same approach as the numbers. Otherwise, you can just cut the 2.0 release without it.

117 commented 3 years ago

Okay I'm going to leave the YYYY-MM-DD strings as-is but add parsing (to UTC) for the full date strings. 😄