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

The date parameter in onChange from DatePickerProps is differently typed in v7.0.0 #4924

Open
arendora opened this issue Jun 19, 2024 · 6 comments

Comments

@arendora
Copy link

Describe the bug
The date parameter in onChange when taken from DatePickerProps is type Date & [Date | null, Date | null] & Date[].

Before v7.0.0 , the date parameter in onChange from ReactDatePickerProps is typed depending on WithRange and WithMultiple so that types Date | null, Date[] | null or [Date | null, Date | null] were accepted.

onChange(
  date: WithRange extends false | undefined
    ? (WithMultiple extends false | undefined ? Date | null : Date[] | null)
    : [Date | null, Date | null],
  event: React.SyntheticEvent<any> | undefined,
): void;

However, in v7.0.0 the date parameter is now type Date & [Date | null, Date | null] & Date[].

To Reproduce
Sandbox link

Expected behavior
onChange from DatePickerProps accepts parameter date of type Date.

Screenshots
ts-date-error

@talatkuyuk
Copy link

talatkuyuk commented Jun 22, 2024

I think there is no bug or problem. It is not Date & [Date | null, Date | null] & Date[] as @arendora described.

The type depends on selectsRange and selectsMultiple.

It is Date | null or [Date | null, Date | null] or Date[] | null depending on selectsRange and selectsMultiple.

(
    | {
        selectsRange?: never;
        selectsMultiple?: never;
        onChange: (
          date: Date | null,
          event?:
            | React.MouseEvent<HTMLElement>
            | React.KeyboardEvent<HTMLElement>,
        ) => void;
      }
    | {
        selectsRange: true;
        selectsMultiple?: never;
        onChange: (
          date: [Date | null, Date | null],
          event?:
            | React.MouseEvent<HTMLElement>
            | React.KeyboardEvent<HTMLElement>,
        ) => void;
      }
    | {
        selectsRange?: never;
        selectsMultiple: true;
        onChange: (
          date: Date[] | null,
          event?:
            | React.MouseEvent<HTMLElement>
            | React.KeyboardEvent<HTMLElement>,
        ) => void;
      }
  );

@arendora
Copy link
Author

@talatkuyuk That was my expectation as well. However, you can see in the sandbox link I provided, selectsRange and selectsMultiple are both undefined/falsy so the date parameter for onChange should be date: Date | null however the typing is instead Date & [Date | null, Date | null] & Date[]

@talatkuyuk
Copy link

talatkuyuk commented Jun 25, 2024

@arendora, don't pass directly onChange of DatePickerProps in Props. Instead, set the type accordingly.

type Props = Omit<DatePickerProps, "onChange"> & {
  label: string;
  onChange: (date: Date | null) => void;
};

export const DatePickerComponent = ({
  label,
  onChange,
  ...datePickerProps
}: Props) => {
  const onChangeHandler = (
    date: Date | null,
    event?: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>
  ) => {
    onChange(date);
  };

  return (
    <label>
      <span>{label}</span>
      <DatePicker {...datePickerProps} onChange={onChangeHandler} />
    </label>
  );
};

@arendora
Copy link
Author

I appreciate the suggestion @talatkuyuk and it is a temporary workaround, however that just overrides the issue and I think the underlying type change should still be addressed.

@talatkuyuk
Copy link

talatkuyuk commented Jun 25, 2024

Thank you @arendora, but, I don't think it is a temporary workaround, it should be as I explained.

You pass a setState function to <DatePickerComponent /> as onChange prop, which is not an event handler for sure, and you shouldn't. I still think there is no issue about the type of onChange in the package. Sorry to say, you use the onChange prop in a wrong way.

If you want to pass strictly an appropriate event handler as a prop of DatePickerProps, define it in App.tsx, and pass it to the DatePicker component.

export default function App() {
  const [startDate, setStartDate] = useState<Date | null>(new Date());

  const onChange = (
    date: Date | null,
    event?: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>
  ) => {
    setStartDate(date);
  };

  return (
    <div className="App">
      <DatePickerComponent
        label="Date"
        onChange={onChange}
        // ...
      />
    </div>
  );
}

In that case, in the component you don't need to destruct onChange which will be in {...datePickerProps}:

type Props = DatePickerProps & {
  label: string;
};

export const DatePickerComponent = ({
  label,
  ...datePickerProps
}: Props) => {
  return (
    <label>
      <span>{label}</span>
      <DatePicker {...datePickerProps}  />
    </label>
  );
};

That is my conclusion.

@jcrang
Copy link

jcrang commented Jul 17, 2024

@arendora The issue you're facing is because TypeScript doesn't know for certain the type should be Date | null. You can see why this is an issue in your CodeSandbox by passing selectsRange={true} to your custom component. The type is inferred to be [Date | null, Date | null] which is incorrectly asserted to be Date in DatePickerComponent's onChangeHandler.

<DatePickerComponent
  selectsRange={true}
  onChange={(d) => {
    /* d: [Date | null, Date | null] */
  }}
  // …
/>

If you'd like to constrain your custom component to only allow single dates you'll need to constrain your props type. One way of doing this is:

type Props = DatePickerProps & {
  label: string;
} & {
  selectsRange?: never;
  selectsMultiple?: never;
};

With this change DatePickerComponent will know for certain that it's working with a single date and the rest of your code will compile – after a small additional change to the first parameter of onChangeHandler to Date | null instead of Date.

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

No branches or pull requests

3 participants