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

format is not pure #41

Closed
TheLudd opened this issue Jun 27, 2016 · 34 comments
Closed

format is not pure #41

TheLudd opened this issue Jun 27, 2016 · 34 comments

Comments

@TheLudd
Copy link
Collaborator

TheLudd commented Jun 27, 2016

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
Copy link
Owner

cullophid commented Jun 27, 2016

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
Copy link
Contributor

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
Copy link
Owner

@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:

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
Copy link
Owner

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
Copy link
Collaborator Author

TheLudd commented Jun 28, 2016

@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
Copy link
Owner

cullophid commented Jun 28, 2016

@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
Copy link
Owner

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

@TheLudd
Copy link
Collaborator Author

TheLudd commented Jun 28, 2016

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
Copy link
Owner

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

@TheLudd
Copy link
Collaborator Author

TheLudd commented Jun 28, 2016

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
Copy link
Owner

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
Copy link
Collaborator Author

TheLudd commented Jun 28, 2016

Perhaps assume 1970 instead?
And day 01
And January

@cullophid
Copy link
Owner

Yeah i think that makes sense.

@davidchambers
Copy link
Contributor

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
Copy link
Owner

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

@cullophid
Copy link
Owner

Hmm purity might come at a high price in this case

@TheLudd
Copy link
Collaborator Author

TheLudd commented Jun 29, 2016

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
Copy link
Contributor

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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Contributor

wuct commented Jun 30, 2016

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

@davidchambers
Copy link
Contributor

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:

  • support UTC only; or
  • integrate with a time-zone database as timezone-js does.

The first option is very limiting.

@svozza
Copy link
Collaborator

svozza commented Jul 1, 2016

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
Copy link
Owner

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

@davidchambers
Copy link
Contributor

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
Copy link
Owner

cullophid commented Jul 4, 2016

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
Copy link
Contributor

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
Copy link
Owner

@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
Copy link
Contributor

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
Copy link
Owner

Sounds good
I think we have a plan:

  • add timezone database (and internal function tz :: String -> Date -> Number)
  • change format, get, isLeapYear, set and possibly parse to accept a String as the first argument specifying the timezone. e.g. format :: String -> String -> Date -> String
  • add impure getSystemTimezone :: -> String
  • conquer the world

@wuct
Copy link
Contributor

wuct commented Jul 6, 2016

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

@cullophid
Copy link
Owner

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
Copy link
Owner

I opened a PR (NOT READY TO BE MERGED)
#54
There are gonna be quite a few things that needs ironing out.
I would love to hear your input

@cullophid
Copy link
Owner

Fixed in version 5.0.2

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

No branches or pull requests

5 participants