Make `parse` safer

[paulyoung]

I was originally doing the following and easily ran into a runtime error because isoString was invalid:

jsDate <- parse isoString
let ms = getTime jsDate

I'm currently doing this instead:

parseJSDate
  :: forall eff
   . String
  -> Eff ( locale :: LOCALE | eff ) (Maybe JSDate)
parseJSDate date = do
  parsed <- parse date
  pure $ if isValid parsed then Just parsed else Nothing

jsDate <- parseJSDate isoString
let ms = getTime <$> jsDate

Is there any reason to not make parse do this by default?

[chexxor]

I agree with having more safe standard functions. 👍
Rename current one to unsafeParse, then introduce a safe parse.

foreign import unsafeParse :: forall eff. String -> Eff (locale :: LOCALE | eff) JSDate
parse :: String -> Eff _ (Maybe JSDate)
-- or --
foreign import parseFFI :: forall a. Maybe a -> (a -> Maybe a) -> String -> Eff _ (Maybe JSDate)
parse :: String -> Eff _ (Maybe JSDate)
parse = parseFFI Nothing Just
-- or --
parse : String -> Eff _ (Maybe JSDate)
parse = do
  parsed <- unsafeParse date
  pure $ if isValid parsed then Just parsed else Nothing

[hdgarrood]

I think we should really be discouraging the use of parse entirely; even if it did return Maybe I don't think it could reasonably be described as "safe", since it's just a wrapper around Date.parse(str). This is not consistent across platforms and its behaviour can also vary unpredictably based on the format of the string you supply; see MDN (which has a warning along the same lines, albeit not very detailed).

However, I think it does make sense to include parse, for cases where people are porting JS code which uses Date.parse to PS (of course, just as a temporary measure).

AFAIK, this package is intended to be nothing more than a thin wrapper over the JS API, and so I think the current behaviour does actually make the most sense. I'd like to update the docs to discourage the use of parse more strongly (in favour of alternative date parsing libraries), but trying to give dates a nice API is the job of datetime.

[thomashoneyman]

I've labeled this as document me so that Harry's comment can make it into the docs:

AFAIK, this package is intended to be nothing more than a thin wrapper over the JS API, and so I think the current behaviour does actually make the most sense. I'd like to update the docs to discourage the use of parse more strongly (in favour of alternative date parsing libraries), but trying to give dates a nice API is the job of datetime.

[maxdeviant]

@thomashoneyman Would you envision this as function-level documentation on parse, or should this be a general note on the entire package?

[hdgarrood]

I think the contents of the README adequately say what this package is for already, so I think it should just be a case of adding some clarity to the function-level docs for parse. Specifically: that it is a wrapper around Date.parse(), that an invalid date value can be returned if the date could not be parsed, and that the function should be avoided if at all possible.

[thomashoneyman]

I agree with @hdgarrood. The README already specifies what the general package is for (and recommends alternatives), so it's the function itself that ought to have a bit more documentation.

[thomashoneyman]

We've addressed this in #25.