cullophid / date-fp

Functional programming date manipulation library
121 stars 17 forks source link

format is not pure #41

Closed TheLudd closed 7 years ago

TheLudd commented 8 years ago

format may return different values depending on who uses it or where.

For me, living in Sweden, format('HH:mm', new Date(0)) === '01:00'. It really should be '00:00'. I suspect that people from different parts of the world will have varying results.

Thoughts?

cullophid commented 8 years ago

new Date() is impure, but format is not. when I call new Date(0) in london I get 1970-01-01T00:00:00.000Z In sweden you should get +1 for CET

davidchambers commented 8 years ago

new Date() is impure, but format is not.

I don't think that's right, @cullophid. Essentially a Date object just wraps a number:

forall n :: Number. new Date(n).valueOf() = n

We can get from Date to String in a pure fashion:

> new Date(0).toISOString()
'1970-01-01T00:00:00.000Z'

We can also extract components from the Date object in a pure fashion:

> new Date(0).getUTCHours()
0

As soon as we use anything time-zone dependent, all bets are off:

> new Date(0).getHours()
16

16 is the value I get in San Francisco, but results will vary based on time zone.

The impurity lies in format('HH:mm'), not in new Date(0). ;)

cullophid commented 8 years ago

@TheLudd @davidchambers I stand corrected :)

new Date(n) is pure, but date.getMonth() isn't... Right? Well thats an issue.

It might make sense to change the lib to use UTC methods instead. That does mean that we need a solid strategy for handling timezones though, since the common case is to use local time.

off the top of my head I see a few possible solutions. We could have each function take a timezone as first param and then do:

1.

const get = D.get("CET")

const month = get('month', new Date(0))
  1. export a "factory function" like
const D = require('date-fp)("CET")

or maybe

const D = require('date-fp/local)("CET")

what do people think?

cullophid commented 8 years ago

forcing the user to explicitly stating the timezone might actually make it simpler to work with js dates. I seem to remember quite a few bugs related to timezones

TheLudd commented 8 years ago

@cullophid

forcing the user to explicitly stating the timezone might actually make it simpler to work with js dates.

Agrees Simple is the correct word here, not easy =)

The reason I discovered the error is that we are migrating from moment to date-fp and I found that the same timestamp was showing differently on different pages. I had just never considered the time zone.

I am not sure what format of your options to use but would it not be easier to work with offset instead of names like "CET"?

cullophid commented 8 years ago

@TheLudd I agree, but I often find that when the decision is between simple and easy, simple is the right decision. Im glad to hear that you are migrating to date-fp. I think this is actually quite a big issue. We cant really make a FP date lib where half of the functions are impure...

I agree that offset would probably be simpler than timezone names.

adding a timezone param doesn't necessarily make the lib that harder to use. You could do something like:

// Local Dates
import * as D = from 'date-fp'
const offset = new Date().getTimezoneOffset()
export default {
  ...D,
  set: D.set(offset),
  get: D.get(offset),
  format: D.format(offset),
  isLeapYear: D.isLeapYear(offset)
}
cullophid commented 8 years ago

I think we are actually only talking about 4 functions get, set, format and isLeapYear.

TheLudd commented 8 years ago

What about parse? I don't think we need to modify the signature but time zone support would have to be added. Parsing 1970-01-01 00:00+00:00 with format YYYY-MM-DD HH:mmZ is unambiguous right? (if Z is the correct token for time zone offset)

But then again, what if time zone is not supplied. Then parse must assume UTC, correct?

cullophid commented 8 years ago

I would say so... I could see some use cases for parsing based on a specified timezone... right 5 functions then...

TheLudd commented 8 years ago

Well, use cases aside if parse is to be pure as well it must return a Date wrapping the same number for the same input no matter from where in the world it is called from. And I am assuming for that to be correct we must either say for which time zone we want to parse the string or always assume the same time zone.

cullophid commented 8 years ago

Well parse could just assume UTC unless an other timezone is specified: parse('YYYY-MM-DD HH:mm:ssZ', '1945-01-01 12:00:00+2')`

Currently parse is not pure, since it assumes the current year if no year details are given.

There seems to be a lot to address here. :)

TheLudd commented 8 years ago

Perhaps assume 1970 instead? And day 01 And January

cullophid commented 8 years ago

Yeah i think that makes sense.

davidchambers commented 8 years ago

I agree that offset would probably be simpler than timezone names.

Wouldn't the user then need to determine whether DST applies to each date?

cullophid commented 8 years ago

@davidchambers... But if we just sat "CET" then THW function still isnt pure...

cullophid commented 8 years ago

Hmm purity might come at a high price in this case

TheLudd commented 8 years ago

The way I see it is this: The functions should be pure. If that makes them cumbersome to use, you can always create new functions wrapping these ones where the new functions defaults to the users current year/time zone etc. But this library should be the base and not assume anything.

davidchambers commented 8 years ago

But if we just sat "CET" then THW function still isnt pure...

What does THW mean?

I don't see a problem with taking a time zone identifier as an argument. We should be able define pure functions with types such as TimeZone -> Date -> String and TimeZone -> Date -> Integer. I don't see any ambiguity with an expression such as this:

getHour('Pacific/Auckland', new Date(0))

Maintaining time-zone tables is a lot of work, though, which is why programming languages usually don't include time-zone databases in their standard libraries.

cullophid commented 8 years ago

@davidchambers THW is danish keyboard autocorrect for the. :)

Would getHour('Pacific/Auckland', new Date(0)) not return a different result depending on timezone ?

I agree with @TheLudd that purity is essential, but I would also want the common use cases to be easy. In most cases people will want to use the local timezone, so we should make it easy to do that. If we use timezoneoffset we can do getHour(Date.now().getTimezoneOffset(), new Date())

cullophid commented 8 years ago

So far I think we all agree that functions should be pure. That means that we will convert the lib to use UTC getter and setter functions. It also means that when parsing a date, we will assume 1970-01-01 00:00:00.000 for the missing parts of the datestring. so parse('DD YYYY mm', '03 2009 34') would become "2009-01-03 00:34:00.000"

The final thing we need to decide is how we manage timezones. We need a solution that means that the functions remain pure, we can support any timezone, and its easy to use for the common case.

wuct commented 8 years ago

+1 for using timezoneOffset Including a time-zone table would increase the size of this lib significantly.

davidchambers commented 8 years ago

Would getHour('Pacific/Auckland', new Date(0)) not return a different result depending on timezone ?

I'm confused. 'Pacific/Auckland' is the time zone, so the system's time zone would be irrelevant.

If we use timezoneoffset we can do getHour(Date.now().getTimezoneOffset(), new Date())

I believe what you're suggesting is this:

//    date :: Date
const date = ...;

//    hour :: Integer
const hour = getHour(date.getTimezoneOffset(), date);

We can't use new Date().getTimezoneOffset() as the time-zone offset of the current moment in time might not be correct for the date in question. Right now in San Francisco, for example, I see this:

> new Date().getTimezoneOffset()
420
> new Date(0).getTimezoneOffset()
480

But what if my program is running in San Francisco but I want to get the hour of date in Auckland time? date.getTimezoneOffset() does not do what I want, and I can't hard-code an offset because Auckland is either +12:00 or +13:00 depending on whether DST is in effect.

It seems to me we have two options:

The first option is very limiting.

svozza commented 8 years ago

I don't see any way around this without including a timezone database. timezone include the whole IANA time zone database yet they manage to make the minified version of their module only 2.7k so it doesn't have to be a big overhead.

cullophid commented 8 years ago

@davidchambers sorry I meant that Pacific/Auckland would return a different result based on daylight saving.

davidchambers commented 8 years ago

I meant that Pacific/Auckland would return a different result based on daylight saving.

The time-zone database would need to be aware of DST.

cullophid commented 8 years ago

true, but if the time-zone database is aware of DST isn't it impure? format('Pacific/Auckland', ...) would return different results depending on when you call it.

I still think that fomat needs to take an offset as a param. we can ofc provide impure helper functions around time-zonese e.g. tz('CET') // => 60

does that make sense?

davidchambers commented 8 years ago

if the time-zone database is aware of DST isn't it impure? format('Pacific/Auckland', ...) would return different results depending on when you call it.

No, it would not. We're providing a Date object as the second argument. This represents a moment in time. Any given moment in time corresponds to exactly one date–time in the Pacific/Auckland time zone.

Going in the other direction is quirky (but still pure). There are date–times in the Pacific/Auckland time zone which correspond to two moments in time. One day each year 02:30 occurs twice: once before and once after the one-hour adjustment. If we are asked to determine the moment in time corresponding to 02:30 local time on such a day, we must arbitrarily (but consistently) choose either the first 02:30 or the second one.

we can ofc provide impure helper functions around time-zonese e.g. tz('CET') // => 60

I don't see how this is helpful. When dealing with date :: Date, the time-zone offset of the current moment in time is irrelevant. It's the time-zone offset of date that's important.

cullophid commented 8 years ago

@davidchambers Ah ofc!

no there is not reason for a timezone helper function if format('Pacific/Auckland', 'YYYY-MM-DD', date) is pure. Perfect! sorry I was a bit slow to catch up.

That sounds like a strategy to me... functions will take a string as the first args, representing the desired timezone.

what do we do for the common case of local time? often we will want to just display what ever timezone the user is in?

davidchambers commented 8 years ago

what do we do for the common case of local time? often we will want to just display what ever timezone the user is in?

We could provide an impure function such as getSystemTimeZone :: -> String. This would allow all the other functions to be pure.

cullophid commented 8 years ago

Sounds good I think we have a plan:

wuct commented 8 years ago

@davidchambers Thanks for explaining the detail. The timezone issue is more complicated then I thought. I have learnt a lot from your comments :)

cullophid commented 7 years ago

Hi guys sorry it has taken me so long to get any thing done on this I have started a new branch utc. I have changed all functions to use the UTC version of date methods So far I have not addressed timezones at all. I am still insure how we are gonna do that excactly... One option is simply to supply a function for translating a date from one timezone to an other and then let the user keep track of what timezone the date is in.

I think i prefer this to adding a timezone argument to every function call for get, set, isLeapYear, etc.

cullophid commented 7 years ago

I opened a PR (NOT READY TO BE MERGED) https://github.com/cullophid/date-fp/pull/54 There are gonna be quite a few things that needs ironing out. I would love to hear your input

cullophid commented 7 years ago

Fixed in version 5.0.2