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 incorrect date validation from dayjs #608

Merged
merged 3 commits into from
May 5, 2022

Conversation

RCout1nho
Copy link
Contributor

What is the problem?

As shown at mui/mui-x#6983 and mui/mui-x#6984, there is a problem validating dates using dayjs.
I noticed that the parse function uses dayjs parse without strict format, causing dayjs to convert any date, even if incorrect, e.g. "99/99/2000" to a correct date, and due to this, all other functions used later (date, isValid, etc) do not have ability to validade dates, as they will always receive a supposedly valid date.

This issue is the same as mui/material-ui#599

What did I do?

  • I changed the type of the local here and here from string to ILocale, because it's the type passed by dayjs
  • Due to the previous change, I changed the places where this.locale was called as string to this.locale?.name, because it's the equivalent
  • I changed parse function return from this.dayjs(value, format, this.locale) to this.dayjs(value, format, this.locale?.name, true), enabling strict mode.

Note

I'm working with material-ui DatePickers, and with my changes, pickers work perfectly

@RCout1nho
Copy link
Contributor Author

Modifications

I've been analyzing it better and I realized that the best way to use localization is really using the localization string, but remembering that this only works if the localization file for the desired language is imported previously.
I changed the types back to string, and kept the strict. I believe it will now pass the tests.

@dmtrKovalenko
Copy link
Owner

The biggest question from me – will it require breaking change? If not - I will be happy to merge it and release right now

@codecov-commenter
Copy link

Codecov Report

Merging #608 (ab8c416) into master (dc9a2ba) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      mui/material-ui#608   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1450      1450           
  Branches       191       191           
=========================================
  Hits          1450      1450           
Impacted Files Coverage Δ
packages/dayjs/src/dayjs-utils.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc9a2ba...ab8c416. Read the comment docs.

packages/dayjs/src/dayjs-utils.ts Outdated Show resolved Hide resolved
packages/dayjs/src/dayjs-utils.ts Outdated Show resolved Hide resolved
@RCout1nho
Copy link
Contributor Author

The biggest question from me – will it require breaking change? If not - I will be happy to merge it and release right now

No, it does not require a breaking change. In fact, after this change, date-io/dayjs will have the same behavior as date-io/datefns, date-io/moment and others, as it will correctly deliver the validated dates and indicating if there are errors.
Do you agree?

@RCout1nho
Copy link
Contributor Author

RCout1nho commented Feb 16, 2022

I even noticed that the LocatizationProvider component of material-ui was causing some of the confusion, as it allows the locale to be passed both as a string and as an object. However, date-io always accepted only as a string, and this caused an error when validating the locale, and made it always use the default locale ("en"). I will report this to the material-ui people.

@chwallen
Copy link

I even noticed that the LocatizationProvider component of material-ui was causing some of the confusion, as it allows the locale to be passed both as a string and as an object. However, date-io always accepted only as a string, and this caused an error when validating the locale, and made it always use the default locale ("en"). I will report this to the material-ui people.

Yes, it was discussed in mui/material-ui#588.

@chwallen
Copy link

With your reverts, this PR is exactly the same as mui/material-ui#599.

@kjellski
Copy link

kjellski commented May 5, 2022

Can you guys merge either mui/material-ui#599 OR mui/material-ui#608 ? Changes are equal... but the fix would be great... 😃 @dmtrKovalenko

@dmtrKovalenko dmtrKovalenko merged commit 2336d76 into dmtrKovalenko:master May 5, 2022
@kjellski
Copy link

kjellski commented May 5, 2022

❤️ @dmtrKovalenko awesome! 👍 thanks a lot!

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

Successfully merging this pull request may close these issues.

5 participants