mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.01k stars 1.24k forks source link

[pickers] Support `AdapterLuxon` with `throwOnInvalid = true` #11853

Open harry-peirse-jfc opened 6 months ago

harry-peirse-jfc commented 6 months ago

Steps to reproduce

Link to live example

Steps:

  1. Run tsc --noEmit in the terminal
  2. Error appears

Current behavior

It is standard practice in Luxon to write the following when invalid dates should throw errors:

import { Settings } from "luxon";

Settings.throwOnInvalid = true;

declare module "luxon" {
  interface TSSettings {
    throwOnInvalid: true;
  }
}

In particular this type override tells Luxon that null won't be returned when parsing DateTimes.

Unfortuantely when this is set, it now breaks the AdapaterLuxon type checking when providing it to the LocalizationProvider. This worked without issues on @mui/x-date-pickers-pro version 6.18.4

Expected behavior

Providing AdapterLuxon to the LocalizationProvider with throwOnInvalid configured to true should not cause type errors.

Context

No response

Your environment

npx @mui/envinfo ``` Browser: Chrome, but issue is browser agnostic System: OS: Linux 6.1 Debian GNU/Linux 12 (bookworm) 12 (bookworm) Binaries: Node: 20.9.0 - /usr/local/bin/node npm: 9.8.1 - /usr/local/bin/npm pnpm: 8.10.2 - /usr/local/share/npm-global/bin/pnpm Browsers: Chrome: Not Found npmPackages: @emotion/react: latest => 11.11.3 @emotion/styled: latest => 11.11.0 @mui/base: 5.0.0-beta.33 @mui/core-downloads-tracker: 5.15.6 @mui/material: latest => 5.15.6 @mui/private-theming: 5.15.6 @mui/styled-engine: 5.15.6 @mui/system: 5.15.6 @mui/types: 7.2.13 @mui/utils: 5.15.6 @mui/x-date-pickers: latest => 6.19.2 @mui/x-date-pickers-pro: latest => 6.19.2 @mui/x-license-pro: 6.10.2 @types/react: latest => 18.2.48 react: latest => 18.2.0 react-dom: latest => 18.2.0 typescript: latest => 5.3.3 ```

Search keywords: DateTime Typescript AdapterLuxon Error Order ID: 60670

michelengelen commented 6 months ago

Hey @harry-peirse-jfc I can confirm this behavior (bug? 🤷🏼). I'll put this on the board for the team to evaluate the effort! Thanks! 🙇🏼

andbjer commented 6 months ago

This issue was introduced in v6.18.7. I believe this is the PR that introduced the issue https://github.com/mui/mui-x/pull/11566

oscar-b commented 2 months ago

This is easily reproduced by deleting for instance the month part (in the textfield) of a <DateTimePicker<DateTime>>

Uncaught Error: Invalid DateTime: invalid input
    at DateTime.invalid (datetime.js:940:13)
    at DateTime.fromJSDate (datetime.js:600:23)
    at AdapterLuxon.getInvalidDate (AdapterLuxon.js:208:77)
    at clearActiveSection (useFieldState.js:158:69)
    at eval (useFieldV6TextField.js:283:9)
    at eval (useEventCallback.js:25:19)
    at handleChange (InputBase.js:396:7)

Or entering a date in the textfield of a <DateTimePicker<DateTime>> which is initialised without a date:

Uncaught Error: Invalid DateTime: unparsable: the input "0002 NaN NaN hh mm" can't be parsed as format yyyy M d HH mm
    at DateTime.invalid (datetime.js:940:13)
    at parseDataToDateTime (datetime.js:185:21)
    at DateTime.fromFormat (datetime.js:890:14)
    at AdapterLuxon.parse (AdapterLuxon.js:229:58)
    at getDateFromDateSections (useField.utils.js:334:16)
    at updateSectionValue (useFieldState.js:210:99)
"@mui/material": "^5.15.16",
"@mui/x-date-pickers": "^7.7.0",
LukasTy commented 2 months ago

@oscar-b @andbjer @harry-peirse-jfc I've created a draft PR with an experiment on how to handle this luxon setting. Consider trying the following package: https://pkg.csb.dev/mui/mui-x/commit/064c2340/@mui/x-date-pickers to test the behavior and provide any feedback if possible.

-"@mui/x-date-pickers": "^7.7.0"
+"@mui/x-date-pickers": "https://pkg.csb.dev/mui/mui-x/commit/064c2340/@mui/x-date-pickers"

The main issue/question is how do we handle these "invalid" thrown dates as currently it seems more as a bug than a feature. Blurring the text field hides the input/query, when the entry is invalid. Also, IMHO, it would be logical to put the field in an error state when the entry is "invalid/thrown".

https://github.com/mui/mui-x/assets/4941090/5a3812c1-3622-4a23-aad4-2005df123728

What would you consider a proper behavior given the Settings.throwOnInvalid = true? I'll remind you that this setting messes up with the expected lifecycle behavior of Fields. We loose the intermediate Invalid date state. 🙈

oscar-b commented 2 months ago

@LukasTy Thanks for looking into this. The whole situation with types in Luxon is.. not optimal. I don't know if you did more research, but here's some interesting tickets: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/64995 https://github.com/DefinitelyTyped/DefinitelyTyped/pull/65078 https://github.com/moment/luxon/issues/1421

Your PR seems to do the best with the situation at hand (only checked your video, haven't tried it myself yet), but losing the date on blur probably won't fly here. I'll probably have to investigate disabling throwOnInvalid in our codebase instead, considering the issues around Luxon wrt this.

LukasTy commented 2 months ago

@oscar-b thank you for sharing the discussion links. I can only add that if luxon went for a separate import path (luxon/strict) approach, that could be even harder to support as opposed to the existing solution.

I'm not a fan of this behavior (throwOnInvalid) from the Picker Adapters perspective, because luxon is the only library/adapter, that I'm of, which has such option. All the other libraries (and even JS Date) just create an object, which can be identified as invalid and we rely on this. Properly supporting luxon.throwOnInvalid means creating special checks and cases just to cater to this one implementation case. 🙈 🤷

Is the lack of support on the @mui/x-date-pickers side for luxon.throwOnInvalid a deal-breaker for anyone? Or can we wait and see what direction the library takes?

oscar-b commented 2 months ago

@LukasTy For our case, I'm reverting to not using throwOnInvalid anymore, so we're good. Hopefully luxon will add support for locally specifying throwOnInvalid overriding the global setting, but I'm not holding my breath.

SaebAmini commented 1 month ago

After a confused long search of why my build was suddenly encountering the below error, I luckily found this thread linked from the documentation page to understand why it's happening.

I'll just paste the error here to help others find this thread from search engines:

<LocalizationProvider dateAdapter={AdapterLuxon}> results in the type error:

Types of construct signatures are incompatible.
  Type 'new ({ locale, formats }?: { formats?: Partial<AdapterFormats> | undefined; locale?: string | undefined; } | undefined) => AdapterLuxon' is not assignable to type 'new (...args: any) => MuiPickersAdapter<DateTime<true>, string>'.
    Construct signature return types 'AdapterLuxon' and 'MuiPickersAdapter<DateTime<true>, string>' are incompatible.
      The types returned by 'date(...)' are incompatible between these types.
        Type 'DateTime<true> | DateTime<false> | null' is not assignable to type 'DateTime<true> | null'.
          Type 'DateTime<false>' is not assignable to type 'DateTime<true>'.
            Type 'false' is not assignable to type 'true'.ts(2419)
LocalizationProvider.d.ts(22, 5): The expected type comes from property 'dateAdapter' which is declared here on type 'IntrinsicAttributes & LocalizationProviderProps<DateTime<true>, string>'
(property) LocalizationProviderProps<DateTime<true>, string>.dateAdapter?: (new (...args: any) => MuiPickersAdapter<DateTime<true>, string>) | undefined
Date library adapter class function.