dmtrKovalenko / date-io

Abstraction over common javascript date management libraries
MIT License
727 stars 90 forks source link

Fix incorrect date validation from dayjs #608

Closed RCout1nho closed 2 years ago

RCout1nho commented 2 years ago

What is the problem?

As shown at https://github.com/mui/mui-x/issues/6983 and https://github.com/mui/mui-x/issues/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?

Note

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

RCout1nho commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

RCout1nho commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

kjellski commented 2 years ago

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

kjellski commented 2 years ago

:heart: @dmtrKovalenko awesome! :+1: thanks a lot!