-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: react types backwards compat #8099
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
Conversation
zachbwh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this!
btw, to address the specific issues I raised here, I would expect to see more changes in the @react-aria packages, for example in @react-aria/overlays/DismissButton
|
Ah, thank you for catching the react-aria packages, I'll update it tomorrow to include those |
|
@zachbwh thanks again, I've updated the PR |
| export const HiddenContext = createContext<boolean>(false); | ||
|
|
||
| export function Hidden(props: {children: ReactNode}): ReactNode { | ||
| export function Hidden(props: {children: ReactNode}): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why JSX.Element here but ReactElement in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some of these, unpkg was down, so I relied on the ts inference
this one is internal, so doesn't matter too much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, though it may have been inconsistent before as well. Just wondering which one we should use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've double checked all the old ones and included direct links to the previously generated output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including link to comment on type https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d2c29760d5939b14a41084d0154efbf45161bcfe/types/react/index.d.ts#L4005
| * not with a mouse, touch, or other input methods. | ||
| */ | ||
| export function FocusRing(props: FocusRingProps): ReactNode { | ||
| export function FocusRing(props: FocusRingProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked the unpkg for this one, apparently it was ReactElement, I'll update a few of the others
https://app.unpkg.com/@react-aria/[email protected]/files/dist/types.d.ts
| * Provides the locale for the application to all child components. | ||
| */ | ||
| export function I18nProvider(props: I18nProviderProps): ReactNode { | ||
| export function I18nProvider(props: I18nProviderProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }); | ||
|
|
||
| export function ClearPressResponder({children}: {children: ReactNode}): ReactNode { | ||
| export function ClearPressResponder({children}: {children: ReactNode}): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * affordance to do so. | ||
| */ | ||
| export function DismissButton(props: DismissButtonProps): ReactNode { | ||
| export function DismissButton(props: DismissButtonProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * and provides a focus scope for the child elements. | ||
| */ | ||
| export function Overlay(props: OverlayProps): ReactNode | null { | ||
| export function Overlay(props: OverlayProps): JSX.Element | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Sets the portal container for all overlay elements rendered by its children. | ||
| */ | ||
| export function UNSAFE_PortalProvider(props: PortalProviderProps): ReactNode { | ||
| export function UNSAFE_PortalProvider(props: PortalProviderProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * like portals, which can cause the React tree and the DOM tree to differ significantly in structure. | ||
| */ | ||
| export function ModalProvider(props: ModalProviderProps): ReactNode { | ||
| export function ModalProvider(props: ModalProviderProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * overlay should be accessible at once. | ||
| */ | ||
| export function OverlayProvider(props: ModalProviderProps): ReactNode { | ||
| export function OverlayProvider(props: ModalProviderProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * form autofill, mobile form navigation, and native form submission. | ||
| */ | ||
| export function HiddenSelect<T>(props: HiddenSelectProps<T>): ReactNode | null { | ||
| export function HiddenSelect<T>(props: HiddenSelectProps<T>): JSX.Element | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * and provides it to all nested React Aria links to enable client side navigation. | ||
| */ | ||
| export function RouterProvider(props: RouterProviderProps): ReactNode { | ||
| export function RouterProvider(props: RouterProviderProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| export function VirtualizerItem(props: VirtualizerItemProps): ReactNode { | ||
| export function VirtualizerItem(props: VirtualizerItemProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * to screen readers. | ||
| */ | ||
| export function VisuallyHidden(props: VisuallyHiddenProps): ReactNode { | ||
| export function VisuallyHidden(props: VisuallyHiddenProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| export function BreadcrumbItem(props: SpectrumBreadcrumbItemProps): ReactNode { | ||
| export function BreadcrumbItem(props: SpectrumBreadcrumbItemProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal type
| } | ||
|
|
||
| export function CalendarBase<T extends CalendarState | RangeCalendarState>(props: CalendarBaseProps<T>): ReactNode { | ||
| export function CalendarBase<T extends CalendarState | RangeCalendarState>(props: CalendarBaseProps<T>): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal type
| } | ||
|
|
||
| export function CalendarCell({state, currentMonth, firstDayOfWeek, ...props}: CalendarCellProps): ReactNode { | ||
| export function CalendarCell({state, currentMonth, firstDayOfWeek, ...props}: CalendarCellProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
| } | ||
|
|
||
| export function CalendarMonth(props: CalendarMonthProps): ReactNode { | ||
| export function CalendarMonth(props: CalendarMonthProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
| } | ||
|
|
||
| export function DatePickerField<T extends DateValue>(props: DatePickerFieldProps<T>): ReactNode { | ||
| export function DatePickerField<T extends DateValue>(props: DatePickerFieldProps<T>): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal
| * or when the trigger unmounts while the dialog is open. | ||
| */ | ||
| export function DialogContainer(props: SpectrumDialogContainerProps): ReactNode { | ||
| export function DialogContainer(props: SpectrumDialogContainerProps): JSX.Element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types | ||
| props | ||
| ): ReactNode { | ||
| ): JSX.Element | ReactElement<any, string | JSXElementConstructor<any>>[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was any https://app.unpkg.com/@react-spectrum/[email protected]/files/dist/types.d.ts#L19 but according to inference, this is more accurate
| * not with a mouse, touch, or other input methods. | ||
| */ | ||
| export function FocusRing(props: FocusRingProps): ReactNode { | ||
| export function FocusRing(props: FocusRingProps): React.ReactElement<unknown, string | React.JSXElementConstructor<any>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are the defaults for the generic arguments right? So you don't need to include them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll double check, but for some reason our build previously expanded like this, so figured it was better to match that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok but that's not necessarily the right thing
devongovett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok whatever, but I don't think matching what we had previously is necessarily the goal here. 🤷
Closes #8092
chore: Explicit module boundary types introduced some breaking changes for people using old versions of typescript or @types/react
This PR reverts v3 and RAC to match what was implicitly defined in our build, but now it's explicit about it.
You can compare the old build either by building locally before that PR or comparing https://unpkg.com/[email protected]/dist/types.d.ts
You can use a diff tool for before and after.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: