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

types: DayPickerProps to use generic constraints #1718

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

Conversation

ongyuxing
Copy link

Context

Fix issue #1583, where passing a union type to the mode prop of DayPicker results in a TypeScript warning that the type of mode is incorrect.

Analysis

TypeScript does not support merging property types from multiple interfaces in a union type.

Solution

I updated the DayPickerProps type definition to include a generic constraint. Additionally, I provided a default type for the generic parameter to avoid breaking changes.

@gpbl
Copy link
Owner

gpbl commented Mar 8, 2023

@ongyuxing this seems the solution we were looking for. Thanks for the help! I'll check the tests which have too much logs to be properly debugged...

@gpbl gpbl linked an issue Mar 8, 2023 that may be closed by this pull request
@gpbl gpbl self-assigned this Mar 8, 2023
@gpbl gpbl changed the title fix: improved DayPickerProps using generic constraints (#1583) types: Improve DayPickerProps using generic constraints (#1583) Mar 8, 2023
@gpbl gpbl changed the title types: Improve DayPickerProps using generic constraints (#1583) types: DayPickerProps to use generic constraints Mar 8, 2023
@ongyuxing
Copy link
Author

ongyuxing commented Mar 8, 2023

@gpbl I tried to fix the issue that onSelect parameter inference is incorrect when mode is a union type by converting onSelect to an intersection type.
But it requires using a utility types library called type-fest that includes a UnionToIntersection type.
Would you recommend using this library and type for my purposes?

@gpbl
Copy link
Owner

gpbl commented Mar 9, 2023

Would you recommend using this library and type for my purposes?

@ongyuxing I'm interested why type-fest would be necessary? The update you propose seems to work. (The failing test is unrelated to this change.)

Also could we add an example file where we see this change working as intended? Like in website/examples.

Thanks!

@ongyuxing
Copy link
Author

@gpbl When the mode type is a union of 'single' | 'multiple', the onSelect type will also be inferred as a union of SelectSingleEventHandler | SelectMultipleEventHandler. This will result in a TypeScript error when passing in onSelect.

To fix this error, the type of onSelect needs to be converted to SelectSingleEventHandler & SelectMultipleEventHandler, allowing TypeScript to correctly merge the types of parameters and return values. For example, the first parameter will be inferred as a union of Date | Date[].

This is an example:

import type {
  DayPickerBase,
  DayPickerDefaultProps,
  DayPickerMultipleProps,
  DayPickerRangeProps,
  DayPickerSingleProps,
  DaySelectionMode,
  SelectMultipleEventHandler,
  SelectRangeEventHandler,
  SelectSingleEventHandler,
} from 'react-day-picker'
import type { UnionToIntersection } from 'type-fest'

interface DayPickerProps<T extends DaySelectionMode = DaySelectionMode>
  extends DayPickerBase {
  mode?: T
  selected?: T extends 'single'
    ? DayPickerSingleProps['selected']
    : T extends 'multiple'
    ? DayPickerMultipleProps['selected']
    : T extends 'range'
    ? DayPickerRangeProps['selected']
    : DayPickerDefaultProps['selected']
  onSelect?: SelectEventHandler<T>
  required?: T extends 'single' ? DayPickerSingleProps['required'] : never
  min?: T extends 'multiple'
    ? DayPickerMultipleProps['min']
    : T extends 'range'
    ? DayPickerRangeProps['min']
    : never
  max?: T extends 'multiple'
    ? DayPickerMultipleProps['max']
    : T extends 'range'
    ? DayPickerRangeProps['max']
    : never
}

type SelectEventHandler<T extends DaySelectionMode = DaySelectionMode> =
  T extends 'single'
    ? SelectSingleEventHandler
    : T extends 'multiple'
    ? SelectMultipleEventHandler
    : T extends 'range'
    ? SelectRangeEventHandler
    : never

declare function DayPicker<T extends DaySelectionMode = 'default'>(
  props: Omit<DayPickerProps<T>, 'onSelect'> & {
    // Directly changing onSelect in DayPickerProps is not recommended as it will break internal references.
    onSelect: UnionToIntersection<SelectEventHandler>
  }
): JSX.Element

const DatePicker = ({ index }: { index: number }) => {
  const modes = ['single', 'multiple', 'range'] as const
  const values = [
    new Date(),
    [new Date()],
    { from: new Date(), to: new Date() },
  ]

  return (
    <DayPicker
      mode={modes[index]}
      selected={values[index]}
      onSelect={(value) => {}}
    />
  )
}

@ongyuxing
Copy link
Author

This is the running result without any modification:
image
This is the running result without modifying the type of onSelect:
image
This is the running result after the modification:
image

@gpbl
Copy link
Owner

gpbl commented Mar 10, 2023

@ongyuxing I see it now – thanks for the clear explanation! So yes, feel free to add the type-fest dependency. This is a great change, looking forward to release it 👍

@ongyuxing
Copy link
Author

@gpbl It is done.🎉

@gpbl
Copy link
Owner

gpbl commented Mar 12, 2023

Thanks @ongyuxing . I'm seeing interesting types errors... I am looking at them.

@gpbl
Copy link
Owner

gpbl commented Mar 12, 2023

Sorry for hi-jacking your PR, I couldn't get actions working on your fork... I added a test and removed the need of type-fest in 6571670 (#1718).

Could we start from this change? What is missing in the test to require the type-fest utility?

@ongyuxing
Copy link
Author

@gpbl The argument for onSelect is currently being inferred as any type. You need to add "strict: true" to the "compilerOptions" in the tsconfig.json for website. Then TypeScript will report an error.

@gpbl
Copy link
Owner

gpbl commented Mar 12, 2023

@gpbl The argument for onSelect is currently being inferred as any type. You need to add "strict: true" to the "compilerOptions" in the tsconfig.json for website. Then TypeScript will report an error.

LetS investigate this in another PR. I think it will fail for other cases too, not related to this change. I remember we tried to set strict mode and it wasn't working well. I cannot find the PR right now.

@gpbl
Copy link
Owner

gpbl commented Mar 18, 2023

OK I want to give it a second try :)
I'm investigating these errors in the test: https://github.com/gpbl/react-day-picker/actions/runs/4387214125/jobs/7824904268#step:6:20

@gpbl
Copy link
Owner

gpbl commented Mar 18, 2023

If I make the onSelect optional, without strict mode the only thing that is failing is:

export default function Example(props: DayPickerProps) {
  return <DayPicker {...props} />;
}

Screenshot 2023-03-18 at 07 59 10

I don't think we can make much progress from here. In the upcoming version we should rethink the component prop considering this case.

@ongyuxing
Copy link
Author

@gpbl DayPickerProps doesn't match the actual DayPicker props, but changing it will break internal references. A less elegant solution is adding a standalone DayPickerProps type for internal use.

I don't think we can make much progress from here. In the upcoming version we should rethink the component prop considering this case.

You're right! If the onSelect type is modified like this:

type SelectEventHandler = <
  T extends DaySelectionMode = DaySelectionMode
>(
  values: {
    day?: T extends 'single' ? Date : never;
    days?: T extends 'multiple' ? Date[] : never;
    range?: T extends 'range' ? DateRange : never;
    selectedDay: Date;
    activeModifiers: ActiveModifiers;
  },
  e: React.MouseEvent
) => void;

It will make writing type definitions much easier.

@ongyuxing
Copy link
Author

Just as an experiment, I added an InternalDayPickerProps type.

@balusio
Copy link

balusio commented Jun 15, 2023

Hello, i'm facing this issue too with dayPicker, do we have any ideas when can have this merged?

@samuelg0rd0n
Copy link

Any updates on this please?

@gpbl gpbl added this to the v9 milestone Jul 13, 2023
@gpbl
Copy link
Owner

gpbl commented Jul 13, 2023

@samuelg0rd0n we plan to implement this in the next mayor version (still WIP), as it may be a breaking change.

@gpbl gpbl marked this pull request as draft July 15, 2023 12:56
@gpbl
Copy link
Owner

gpbl commented Jul 15, 2023

Please checkout this proposal for generic types in DayPickerProps.

Any feedback?

@AntoineKM
Copy link

Any news here ?

@XXXVII
Copy link

XXXVII commented Apr 16, 2024

Any updates on this one?

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

Successfully merging this pull request may close these issues.

types: investigate type error with mode prop
6 participants