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

Date validator request #204

Open
jathanasiou opened this issue Nov 19, 2019 · 18 comments
Open

Date validator request #204

jathanasiou opened this issue Nov 19, 2019 · 18 comments

Comments

@jathanasiou
Copy link
Contributor

This package is currently not providing a Date validator, which would be very useful for validating date-type inputs. Such a validator is already provided by ember-validators but is not in this project.

https://github.com/offirgolan/ember-validators/blob/v2.0.0/addon/date.js

Having it available would allow us to use changesets for date inputs with something better than

validateFormat({ regex: /[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]/ })

which is currently the best, but somewhat hacky, implementation option.

@snewcomer
Copy link
Collaborator

I think this would be a great idea! Would happily accept a PR!

@jathanasiou
Copy link
Contributor Author

@snewcomer Done, let me know if it works for you.

@jelhan
Copy link
Contributor

jelhan commented Feb 2, 2020

@snewcomer Thanks a lot for implementing the date validator in #218. Can't count how often I have implemented something like this in app space. I should have pushed one of them upstream. 🤦‍♂️

I'm wondering why you decided to not support strings. I often used a date validator directly together with <input type="date"> and other date input types. This would mean to support a limited range of strings:

  • yyyy-MM-dd (date)
  • yyyy-MM-ddThh:mm with optional seconds and milliseconds (datetime-local)
  • yyyy-Www (week)
  • yyyy-MM (month)
  • yyyy (year)

Would you accept a pull request implementing such a support?

@snewcomer
Copy link
Collaborator

👋 Good question. So I thought it might be good to not allow strings for 2 reasons.

  1. Creating dates with a string is a source of error if the string doesn't contain time information.
  2. If you are using a library like moment, the dates could be constructed as Moment and passed in as arguments (assuming moment('02-01-2019') <= moment('01-01-2019') works...does it?)

What are your thoughts? Is the current validator somewhat flexible enough for your use case?

@jelhan
Copy link
Contributor

jelhan commented Feb 3, 2020

Creating dates with a string is a source of error if the string doesn't contain time information.

You could also see it the other way around: Information about the date is lost on conversion from ISO8601 string to a Date object. As far as I know you can't tell from the date object if

  • it included a timezone or was in locale time (yyyy-mm-ddThhThh:mm:ss+00:00 vs yyyy-mm-ddThhThh:mm:ss)
  • included time information (yyyy-mm-ddThh:mm:ss) or way day-only (yyyy-mm-ddThh) or for a specific calendar week (yyyy-Www), month (yyyy-mm) or even only about a year (yyyy).

In most cases the date should include time and time zone information but for some dates this conversion could be wrong. E.g. new year is independent of time zone. It was at 2020-01-01T00:00 this year for everyone regardless of the time zone. But the new year was celebrated in New York at 2020-01-01T00:00-05:00.

Sometimes I prefer to not convert the ISO 8601 date strings to not loose that information.

If you are using a library like moment, the dates could be constructed as Moment and passed in as arguments (assuming moment('02-01-2019') <= moment('01-01-2019') works...does it?)

Not sure about moment to be honest but for native Date this is working cause it implements valueOf(), which returns the timestamp. So new Date('2010-01-01') < new Date('2010-02-02') is the same as new Date('2010-01-01').valueOf() < new Date('2010-02-02').valueOf(), which is the same as 1262304000000 < 1265068800000.

Is the current validator somewhat flexible enough for your use case?

I think it's flexible enough but I don't like that I need to convert the date string to a Date object only for validation purposes.

@snewcomer
Copy link
Collaborator

I think you have brought up some good points. I would also note...

prefer to not convert the ISO 8601 date strings to not loose that information

Either we convert it to a Date object or we ask you to do so. Somebody has to do it so we can do propert comparisions. I would just prefer that we are library independent. moment, luxon, date-fns, dayjs will help with these normalization issues common to JS apps.

My concerns revolve around the variety of permutations the strings may come in as you have noted. The date from the input might be different string format than the date based to before.

new Date('2020-01-01T10:00:00Z')
new Date('2020-01-01T10:00:00')

Any other thoughts or ideas around supporting strings?

@jelhan
Copy link
Contributor

jelhan commented Feb 4, 2020

I understand your concerns but to be honest I think most of them are caused by relying on Date objects rather than using ISO8601 strings.

I think this could be implemented safely without introducing performance issues if having three restrictions:

  1. Validated value and options must use the same type. If the validated value is a Date object, all options must be Date objects as well. If the value is a string all options must be strings as well.
  2. If the value is a string, it must be an ISO 8601 string that is an allowed value for an input element.
  3. If the value is an ISO 8601 string, the same format must be used for value as for options.

The first restriction ensures that we don't have to deal with date parsing at all. The second one limits the possible formats to a small set. The third one allows us to do a simple string comparison.

If I didn't missed something the implementation should be straight forward. To verify that value and options are using the same format comparing the length of the strings should be enough. ISO 8601 spec ensures that two dates of the same format could be compared as strings.

Not sure if it should also verify that it's a valid date string. E.g. either throw or fail validation if value is 2020-02-31. I think such a check would require parsing the date to not deal with all that complexity ourselves. But if it's only an assertion or optional feature it would not have a performance impact.

@snewcomer
Copy link
Collaborator

I could be on board with string comparison! And I agree length should be enough (although - or / would give us a false positive). I'm not sure we need to only limit to ISO8601 either (so <input type="date"> works). What do you think? I'd be fine as long as we have some sort of string validator...

@jelhan
Copy link
Contributor

jelhan commented Feb 6, 2020

I'm not sure we need to only limit to ISO8601 either

Not limiting to a subset of ISO 8601 opens up a wide range of edge cases. The spec allows so much different ways to express the same information. Also I don't see a use case. Consumers either validated the return of a native input field or of a library. In the later case it's much likely that the library either returns a Date object or one of the ISO 8601 formats that are used by web platform. Beside the formats used by native inputs this is only yyyy-mm-ddThh:mm:ss.ssssT, which is used by Date.toISOString().

Not limiting to ISO 8601 would allow formats for which string comparison does not work. E.g. a consumer may use it with German locale format, which is dd.mm.yyyy.

What string formats do you like to support additionally?

Edit: Just noticed that this may be caused by confusing the format returned by Date.toISOString() with the spec itself. That method is only using one format out of a huge set of possibilities. Wikipedia lists some of them: https://en.m.wikipedia.org/wiki/ISO_8601 All formats used by native HTML inputs are specified by ISO 8601 spec.

@snewcomer
Copy link
Collaborator

Ah I assumed the value when mutating an input[date] was not iso8601, but if it is, then 👍 . Would you be willing to put up a PR with a test / dummy example with the native input and we can start from there?

@snewcomer
Copy link
Collaborator

@jelhan ping! Lmk if you would like me to take it on! I got some time this week.

@jelhan
Copy link
Contributor

jelhan commented Apr 26, 2020

Lmk if you would like me to take it on! I got some time this week.

This would be awesome. I'm totally overloaded with Open Source stuff currently. 😢

@steveszc
Copy link
Contributor

steveszc commented Jan 8, 2021

It'd be great to allow the before, onOrBefore, after, onOrAfter options accept a function that returns a date. This way every time the validator is executed we can re-evaluate those values instead of evaluating only when the validation is defined. This is useful if we need to use a value that may change as the value for before, onOrBefore, after, onOrAfter (ex other date properties on the changeset).

I'd be happy to PR this change if there is agreement here.

@jelhan
Copy link
Contributor

jelhan commented Jan 27, 2021

@steveszc Make sense. But wondering if this is needed on all validators. Or is date validator a special case?

@snewcomer
Copy link
Collaborator

I think if we started with the date validator, that would be a great starting point! Lmk if you have any questions @steveszc

@steveszc
Copy link
Contributor

steveszc commented Feb 8, 2021

@snewcomer just put up a PR adding the function support for date validator options.
#281

@snewcomer
Copy link
Collaborator

Thank you!! 3.13.0

@snewcomer
Copy link
Collaborator

@jelhan Do you have an interest in taking a look at this PR? I did it sort of naively. But the plan would be to use this here if it fit your use case!

adopted-ember-addons/ember-validators#100

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

Successfully merging a pull request may close this issue.

4 participants