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

Date.fromString should not rely on new Date(...) #632

Closed
Zimmi48 opened this Issue May 30, 2016 · 9 comments

Comments

Projects
None yet
6 participants
@Zimmi48

Zimmi48 commented May 30, 2016

Hello,
The following SSCCE shows an unexpected result 2011 when we should instead display Error:

import Date
import Html


main =
  Date.fromString "14/01/2010"
  |> Result.map (Date.year >> toString)
  |> Result.withDefault "Error"
  |> Html.text

(tried in elm-lang.org/try in Firefox)

This problem does not arise in elm repl:

> import Date
> Date.fromString "14/01/2010" |> Result.map (Date.year >> toString) |> Result.withDefault "Error"
"Error" : String
@halfzebra

This comment has been minimized.

Show comment
Hide comment
@halfzebra

halfzebra May 31, 2016

Contributor

Date constructor in JavaScript offers consistent parsing behavior only for ISO 8601 formatted date strings.

You have happened to use an invalid ISO 8601 date string, which leads to that error.

Maybe it would make sense to add some validation mechanism to Native.Date.fromString to ensure, that Date constructor will only get a valid ISO 8601 formatted date.

E.g. JavaScript implementation of http://www.pelagodesign.com/blog/2009/05/20/iso-8601-date-validation-that-doesnt-suck/

Contributor

halfzebra commented May 31, 2016

Date constructor in JavaScript offers consistent parsing behavior only for ISO 8601 formatted date strings.

You have happened to use an invalid ISO 8601 date string, which leads to that error.

Maybe it would make sense to add some validation mechanism to Native.Date.fromString to ensure, that Date constructor will only get a valid ISO 8601 formatted date.

E.g. JavaScript implementation of http://www.pelagodesign.com/blog/2009/05/20/iso-8601-date-validation-that-doesnt-suck/

@Zimmi48

This comment has been minimized.

Show comment
Hide comment
@Zimmi48

Zimmi48 May 31, 2016

@halfzebra Thanks for the feedback.
I get what you mean and yes, I think it would make sense to add a validation mechanism, or even better, re-implement the function in pure Elm: a well-behaved language such as Elm shouldn't rely on inconsistent implementations in JS. If a function behaves in some way in the repl, it should behave in the same way in the browser.

Zimmi48 commented May 31, 2016

@halfzebra Thanks for the feedback.
I get what you mean and yes, I think it would make sense to add a validation mechanism, or even better, re-implement the function in pure Elm: a well-behaved language such as Elm shouldn't rely on inconsistent implementations in JS. If a function behaves in some way in the repl, it should behave in the same way in the browser.

@Zimmi48

This comment has been minimized.

Show comment
Hide comment
@Zimmi48

Zimmi48 May 31, 2016

To confirm that this is a problem imported from JS: on Chromium, the same program displays Error

Zimmi48 commented May 31, 2016

To confirm that this is a problem imported from JS: on Chromium, the same program displays Error

@avh4

This comment has been minimized.

Show comment
Hide comment
@avh4

avh4 Jun 1, 2016

Member

If someone can implement validation or pure-elm parsing and merge it to rluiten/elm-date-extra or another published package, that would be a good start.

Member

avh4 commented Jun 1, 2016

If someone can implement validation or pure-elm parsing and merge it to rluiten/elm-date-extra or another published package, that would be a good start.

@evancz evancz changed the title from Problem with date parsing to new Date(...) works different in different browsers Jun 25, 2016

@mariosangiorgio

This comment has been minimized.

Show comment
Hide comment
@mariosangiorgio

mariosangiorgio Aug 27, 2016

Contributor

Hello, is there any update on this issue? I just got confused by the formats used by Date.fromString and opened #701 (sorry for the duplicate)

Contributor

mariosangiorgio commented Aug 27, 2016

Hello, is there any update on this issue? I just got confused by the formats used by Date.fromString and opened #701 (sorry for the duplicate)

@halfzebra

This comment has been minimized.

Show comment
Hide comment
@halfzebra

halfzebra Aug 29, 2016

Contributor

Date.Extra.fromIsoString is capable of parsing ISO 8601 date strings, without causing run-time errors.

I do agree with everything, from #701, Date.fromString should have some extra validation.

Contributor

halfzebra commented Aug 29, 2016

Date.Extra.fromIsoString is capable of parsing ISO 8601 date strings, without causing run-time errors.

I do agree with everything, from #701, Date.fromString should have some extra validation.

@evancz evancz changed the title from new Date(...) works different in different browsers to Date.fromString should not rely on new Date(...) Sep 23, 2016

@daniellandau

This comment has been minimized.

Show comment
Hide comment
@daniellandau

daniellandau Oct 6, 2016

I just ran into a problem with this in production. My situation is that I rely on Date.fromString to fail on invalid input so that Json.oneOf tries the next decoder in the list. In Firefox this works perfectly, but Chrome/Chromium happily give a nonsensical but valid date with for example new Date("3")

daniellandau commented Oct 6, 2016

I just ran into a problem with this in production. My situation is that I rely on Date.fromString to fail on invalid input so that Json.oneOf tries the next decoder in the list. In Firefox this works perfectly, but Chrome/Chromium happily give a nonsensical but valid date with for example new Date("3")

@halfzebra

This comment has been minimized.

Show comment
Hide comment
@halfzebra

halfzebra Oct 6, 2016

Contributor

@daniellandau for now you can use Json.Decode.andThen to parse a string firs and then do Date.Extra.fromIsoString to parse it properly.

Contributor

halfzebra commented Oct 6, 2016

@daniellandau for now you can use Json.Decode.andThen to parse a string firs and then do Date.Extra.fromIsoString to parse it properly.

@lukewestby lukewestby referenced this issue Jan 20, 2017

Closed

Date #816

2 of 7 tasks complete
@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby Jan 20, 2017

Member

Tracking in #816

Member

lukewestby commented Jan 20, 2017

Tracking in #816

@lukewestby lukewestby closed this Jan 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment