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

fix(date)!: Restore 'format', 'precison', and usage of moment for the date validator #726

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

tehhowch
Copy link

@tehhowch tehhowch commented Oct 8, 2022

Resolves #723 .

Changes proposed:

I still think it would be better to support a consumer-defined "use luxon" or "use moment" or "use ___", or perhaps to propose an interface in which the consuming app can massage their preferred date library to support determining the before / onOrBefore / after / onOrAfter relationship states.

  • restores the test suite semantics from before Updated ember required and ember validators packages #715 (git diff d6c95d741fa8388ae2c764037bddcd6c91691f09 tests/unit/validators/date-test.js), while adding some additional tests / ensuring the test assertion messages help convey the test intent.

Tasks:

  • Added test case(s)
  • Updated documentation

cc: @fsmanuel for visibility

 - add explicit test for "wrongDateFormat" being raised when options.format is given and not satisfied
…d by ember-validators

 - the version from ember-validators does not support "now" or "precision" or "format" correctly, and is unlikely to change to support them
 - indicate that `moment` should be installed in the consuming application, but it does not need to be provided by this package
   - TODO: allow configuring the date library used, e.g. provide a moment validator, a luxon validator, etc, and allow the consuming applications to indicate which kind should be used.
(This can show up in debounced input scenarios, e.g. where a user is editing both a start and an end date.)
@tehhowch tehhowch changed the title fix(date): Restore 'format', 'precison', and usage of moment for the date validator fix(date)!: Restore 'format', 'precison', and usage of moment for the date validator Oct 11, 2022
@tehhowch
Copy link
Author

It's worth noting that this is a breaking change to only those on 4.0.0-beta.13 or v4 that modified their usage of the date validator to be compatible with the version provided in ember-cp-validations.

Anyone who is still on 4.0.0-beta.12 or lower would probably consider this to remove a breaking change that is currently in v4.

@kiwi-josh
Copy link

I still think it would be better to support a consumer-defined "use luxon" or "use moment"

@tehhowch I very much agree with this.

We are currently running the qonto@3767e41 in production (as we made efforts to upgrade to ember 4 prior to this addon being adopted).

We have also recently replaced momentjs with dayjs (saving ~25kB's of payload size).

Therefore this PR would be a step backwards for us getting back onto the official repo, as we wouldn't want the extra kB's being reintroduced.

I also completely understand that this PR would undo some unexpected breaking changes for consuming apps that rely on now / precision etc, which I'm all for - this was more a "I agree with your suggestion to let the consumer decide"

@fsmanuel
Copy link
Contributor

@tehhowch I'm on holiday and will have a look after I'm back. I'm still undecided how to move forward with the date validator. I think the addon should not diverge/reimplement too much from ember-validators. Maybe we can provide the old implementation (from this PR) in the docs/readme/migration guide. That way people can use it if they want.

I also agree that a more agnostic solution would be great. Next to moment and dayjs there is also date-fns

 - move new code to "date-moment" validator
 - TODO: ensure moment is actually optional
 - TODO: same approach for luxon
@tehhowch tehhowch marked this pull request as draft October 16, 2022 14:56
Ideally we could just link to ember-validators, but they do not offer upgrading guidance

 - Some examples need to be fleshed out further
since optionalDependencies will be attempted for install
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants