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

[Feature Request] Restore date validator capabilities re: 'format' and 'precision,' and allow use of 'now' as a reference time #723

Open
tehhowch opened this issue Sep 14, 2022 · 8 comments · May be fixed by #726

Comments

@tehhowch
Copy link

The use of ember-validators >4 in #715 removed the ability to specify 'now' and 'precision' unnecessarily, and presents a huge pain point for consumers that generate their validations at the time of bundle/startup. (#715 failed to reflect these changes in the documentation site, too, so consumers can be misled rather easily:
image

Given that this addon wraps ember-validators, it should be simple for the Date validator to handle receiving 'now' and 'precision' arguments and construct the correct calls to make to ember-validators.

For example, if the validator is created with 'now', then any time the validator runs, it would compute the current datetime and forward that to ember-validator, rather than just passing down 'now'.

With support for these two arguments restored, a large breaking change that requires consumer apps to rearchitect simply vanishes, making deprecation removal and upgrading easier for the ecosystem.

cc @fsmanuel for visibility

@fsmanuel
Copy link
Contributor

fsmanuel commented Sep 14, 2022

@tehhowch I'm not familiar with date validations. Can you post some examples how you use them? Maybe we can figure out how to mitigate the pain.

For reference the PR that introduced the changes: adopted-ember-addons/ember-validators#100

@tehhowch
Copy link
Author

I saw that PR, yes. (I also noticed that the changes to make the tests work completely ignored the point and purpose of some of the tests, e.g. every test around 'now' was reimplemented without changing the name-i.e. purpose-of the test, e.g. https://github.com/rwwagner90/ember-validators/pull/100/files?diff=split&w=0#diff-1b3840443c6b8c137c8790bb3a5e573b85ea331871438650849e8abf7b073d5fR120-R134)

The primary use case my team runs into is a validator that is defined in an internal addon that uses 'now', e.g.

const endDateInFutureValidation = {
  onOrAfter: 'now',
  format: SOME_FORMAT,
  errorFormat: OTHER_FORMAT,
  messageKey: 'validations.end.date.in.past',
};
// ...
const Validations = buildValidations({
  // ... other validatons
  endDate: validator('date', {
    disabled: or('..conditions'),
    ...endDateInFutureValidation,
  }),
})

and the goal is that users can't schedule something that is in the past. The upstream PR replaces usage of 'now' with explicit calls to the current time (example), which has no guarantee in production code that it is still the current time when the validation is actually performed, especially if the user has a long session.

@fsmanuel
Copy link
Contributor

fsmanuel commented Sep 14, 2022

I agree that the tests around the date validations got messy and should be revisited. Also the docs need an update.

Something I run into doing the updates was the fact that it's possible to have getter as options.

validator('date', {
before: new Date(),
get after() {
return moment().subtract(120, 'years').format('M/D/YYYY');
},

So in your case

const endDateInFutureValidation = {
  get onOrAfter() { 
    return new Date();
  }, 
  format: SOME_FORMAT,
  errorFormat: OTHER_FORMAT,
  messageKey: 'validations.end.date.in.past',
};

should work.

@tehhowch
Copy link
Author

That could indeed work, but as I was investigating, I determined that PR to ember-validators also broke the expectations on format - namely, that the input date should be in that format, and if not, a wrongDateFormat error should be raised.

To unblock my team's apps, we have just completely stopped using the 'date' validator shipped by this repo/ember-validators, because using it would have required changing how we do 'now', 'precision', and also required inserting another validator to handle required formats.

(There was also the issue that the upstream PR defaults all input parsing of dates to new Date(<string>), which is well-documented as something you should avoid due to the wide variety of implementation behaviors.)

@fsmanuel
Copy link
Contributor

@tehhowch Did you use an inline validator to replace the date validator? Would be great if we can document how people in the same situation can solve the problem...

@tehhowch
Copy link
Author

I extended the existing date validator and provided a fixed validate method that still used moment internally.

I think what could make sense is to provide two peer/optional dependencies, one for luxon and one for moment, and use the test method to have the 'date' validator use whichever has also been installed rather than the busted ember-validator version.

@fsmanuel fsmanuel pinned this issue Sep 17, 2022
@tehhowch tehhowch changed the title [Feature Request] Restore ability to use 'now' for date validators [Feature Request] Restore date validator capabilities re: 'format' and 'precision,' and allow use of 'now' as a reference time Sep 18, 2022
@fsmanuel
Copy link
Contributor

@tehhowch do you mind sharing your solution here so others can use it?

@tehhowch
Copy link
Author

I'll need to reimplement it blind, so no promises on when I'll have free time to do that.

I also think there is perhaps more value to the approach I mentioned before, where library-specific validators (one for luxon, one for moment, etc.) are implemented and conditionally used. That would require the consumers to configure the add-on.

(Possibly those validators / customization belong in ember-validators instead, and we run into the issue of not wanting to auto-install all date libraries.)

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