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

Arguments to callback prop of generic React component are not inferred #44596

Closed
Mesoptier opened this issue Jun 15, 2021 · 20 comments · Fixed by #49707
Closed

Arguments to callback prop of generic React component are not inferred #44596

Mesoptier opened this issue Jun 15, 2021 · 20 comments · Fixed by #49707
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@Mesoptier
Copy link

Bug Report

🔎 Search Terms

react, polymorphic components, generic components, type inference on function arguments

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about type inference on function arguments

⏯ Playground Link

Playground link with relevant code

💻 Code

import React from 'react';

type Props<T extends React.ElementType> =
  { as: T } & Omit<React.ComponentPropsWithoutRef<T>, 'as'>;

declare function Button<T extends React.ElementType>(props: Props<T>): JSX.Element;

// Using Button as React component (intended use case):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
<Button
  as="button"
  // OK: The type of `onClick` is `(JSX attribute) onClick?: React.MouseEventHandler<HTMLButtonElement> | undefined`.
  // NOT OK: The type of `e` is `any` (implicitly).
  onClick={(e) => { console.log(e); }}
/>;

// Using Button as regular function (same issue):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
Button({
  as: 'button',
  // OK: The type of `onClick` is `(property) onClick?: React.MouseEventHandler<HTMLButtonElement> | undefined`.
  // NOT OK: The type of `e` is `any` (implicitly).
  onClick: (e) => { console.log(e); },
});

🙁 Actual behavior

The argument e to the callback function provided to onClick implicitly has any type.

🙂 Expected behavior

I would expect the checker to know that the type of e can only be React.MouseEvent<HTMLButtonElement, MouseEvent>.

The checker seems to know that the type of the onClick callback is React.MouseEventHandler<HTMLButtonElement>, which in turn resolves to (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void. So I would expect the checker to be able to use that information to determine the type of e / event.

@Mesoptier
Copy link
Author

I've looked into this some more. If I make the onClick prop required (i.e. remove the | undefined), the inference starts working correctly. However, this is obviously not desirable, since the onClick prop is optional for a reason.

Playground link with relevant code

import React from 'react';

type MakeRequired<T, K> = K extends keyof T ? Omit<T, K> & Required<Pick<T, K>> : T;

type Props<T extends React.ElementType> =
  { as: T } & MakeRequired<Omit<React.ComponentPropsWithoutRef<T>, 'as'>, 'onClick'>;

declare function Button<T extends React.ElementType>(props: Props<T>): JSX.Element;

// Using Button as React component (intended use case):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
<Button
  as="button"
  // OK: The type of `onClick` is `(JSX attribute) onClick: React.MouseEventHandler<HTMLButtonElement>`.
  // OK: The type of `e` is `(parameter) e: React.MouseEvent<HTMLButtonElement, MouseEvent>`.
  onClick={(e) => { console.log(e); }}
/>;

// Using Button as regular function (same issue):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
Button({
  as: 'button',
  // OK: The type of `onClick` is `(property) onClick: React.MouseEventHandler<HTMLButtonElement>`.
  // OK: The type of `e` is `(parameter) e: React.MouseEvent<HTMLButtonElement, MouseEvent>`
  onClick: (e) => { console.log(e); },
});

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Jun 15, 2021
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Jun 15, 2021
@RyanCavanaugh
Copy link
Member

We can take a look earlier if someone can remove the react dependency from the repro, since it's entirely possible there's something in the definition that causes this per expected behavior.

@Mesoptier
Copy link
Author

After reading your comment I also searched through the issue tracker on DefinitelyTyped. There I found this issue for @types/react which looks similar to what I'm describing: DefinitelyTyped/DefinitelyTyped#50459. According to that, this might an issue with the React typings. However, I'll continue trying to reproduce the issue without the react dependency.

@Mesoptier
Copy link
Author

Mesoptier commented Jun 15, 2021

I've altered my original code sample to no longer include the react dependency. Instead it now includes a few minimal types based on those found in @types/react, which is still enough to demonstrate the issue somewhat succinctly. Hopefully this will help in narrowing down on the issue.

Playground link

Interestingly, if I comment line 19 (| ElementConstructor<P>), the issue disappears.

Playground link with relevant line commented

I can see how this might suggest that the issue lies within @types/react. However, the fact remains that TypeScript seems to be able to correctly resolve the type of the onClick callback (including its event argument), but not propagate that information to the type of the actual argument e in the provided callback.


Edit: I see now that the JSX namespace is no longer included, leading to some squiggly red error lines. Adding import React from 'react'; should fix those. Then the dependency is only used for TypeScript to understand the JSX part and does not impact the issue described in any way.

@Mesoptier
Copy link
Author

Looking at this comment from earlier, I realised that it probably wasn't the "making the prop required" part of it all that made it work. I removed the Required<> utility type, and kept only the Pick<> and it still seems to work.

- type MakeRequired<T, K> = K extends keyof T ? Omit<T, K> & Required<Pick<T, K>> : T;
+ type FixCallback<T, K> = K extends keyof T ? Omit<T, K> & Pick<T, K> : T;

More interestingly, applying this fix to the onClick handler only will also make the onBlur event handler work in the function case, but NOT in the JSX case!

Playground link

import React from 'react';

type FixCallback<T, K> = K extends keyof T ? Omit<T, K> & Pick<T, K> : T;

type Props<T extends React.ElementType> =
  { as: T } & FixCallback<Omit<React.ComponentPropsWithoutRef<T>, 'as'>, 'onClick'>;

declare function Button<T extends React.ElementType>(props: Props<T>): JSX.Element;

// Using Button as React component (intended use case):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
<Button
  as="button"
  // OK: The type of `onClick` is `(JSX attribute) onClick: React.MouseEventHandler<HTMLButtonElement>`.
  // OK: The type of `e` is `(parameter) e: React.MouseEvent<HTMLButtonElement, MouseEvent>`.
  onClick={(e) => { console.log(e); }}
  // NOT OK: The type of `e` is `any`.
  onBlur={(e) => { console.log(e); }}
/>;

// Using Button as regular function (same issue):

// OK: The type of `Button` is `function Button<"button">(props: Props<"button">): JSX.Element` in this instance.
Button({
  as: 'button',
  // OK: The type of `onClick` is `(property) onClick: React.MouseEventHandler<HTMLButtonElement>`.
  // OK: The type of `e` is `(parameter) e: React.MouseEvent<HTMLButtonElement, MouseEvent>`.
  onClick: (e) => { console.log(e); },
  // OK (Somehow?!): The type of `e` is `(parameter) e: React.FocusEvent<HTMLButtonElement>`.
  onBlur: (e) => { console.log(e); },
});

@quantizor
Copy link

@RyanCavanaugh I see that this is marked as backlog. A lot of people in the FE world use this prop inference pattern... would be good to at least target a milestone for fixing it.

@devanshj
Copy link

We can take a look earlier if someone can remove the react dependency from the repro, since it's entirely possible there's something in the definition that causes this per expected behavior.

Here's the react free version @RyanCavanaugh asked for. Playground. (Uses react import just for the JSX.IntrinsicElements definition).

import React from "react";

declare const Identity:
  <C extends keyof JSX.IntrinsicElements>
    (props: { as?: C } & JSX.IntrinsicElements[C]) => 
      JSX.Element

<Identity
  as="input"
  onChange={value => {
    // value is any, expected string
  }}
  value="foo" />

Identity({
  as: "input",
  onChange: value => {
    // value is string, expected string
  },
  value: "foo"
})

@Mesoptier
Copy link
Author

We can take a look earlier if someone can remove the react dependency from the repro, since it's entirely possible there's something in the definition that causes this per expected behavior.

@RyanCavanaugh OK, I now have an actual react-free reproduction case for this issue. One thing to note is that I've set TypeScript's JSX option to 'Preserve', to avoid errors from using JSX syntax without a React import.

Playground link

interface Elements {
  foo: { callback?: (value: number) => void };
  bar: { callback?: (value: string) => void };
}

type Props<C extends keyof Elements> = { as?: C } & Elements[C];
declare function Test<C extends keyof Elements>(props: Props<C>): null;

<Test
  as="bar"
  callback={(value) => {
    // expected: typeof value = string
    // actual: typeof value = any (implicitly)
    // NOT OK
  }}
/>;

Test({
  as: "bar",
  callback: (value) => {
    // expected: typeof value = string
    // actual: typeof value = string
    // OK
  },
});

<Test<'bar'>
  as="bar"
  callback={(value) => {
    // expected: typeof value = string
    // actual: typeof value = string
    // OK
  }}
/>;

It still seems so weird to me that the type of the callback is correct, but the type of the callback parameter is not:

image
image

@RyanCavanaugh
Copy link
Member

@Mesoptier thanks, that's a great repro

@MartinJohns
Copy link
Contributor

#44596 (comment) works in 4.8.2, but the original code example by @Mesoptier still does not work.

@gynekolog
Copy link

The issue in the original code comes with the Omit in the Props definition.
I haven't solution for the issue but I can share playground link with the original code extended by Props without Omit.

@cristatus
Copy link

I have a similar case and it's still not working:

import React from 'react'

interface AsComponent<E extends React.ElementType> {
  <As extends React.ElementType = E>(props: {as?: As} & React.ComponentProps<As>): JSX.Element | null
}

declare const Button: AsComponent<"button">

export const Test = () => {
  return (
    <div>
      <Button onClick={e => {}}>Button</Button>
      <Button onClick={e => {}} as="a">Link</Button>
    </div>
  )
}

See the playground link.

@cristatus
Copy link

I fixed my issue like this:

import React from 'react'

interface AsComponent<P, E extends React.ElementType<P>> {
  (props: React.ComponentProps<E>): JSX.Element | null;
  <As extends React.ElementType<P> = E>(props: { as?: As } & React.ComponentProps<As>): JSX.Element | null
}

declare const Button: AsComponent<{}, "button">

export const Test = () => {
  return (
    <div>
      <Button onClick={e => { }}>Button</Button>
      <Button onClick={e => { }} as="a">Link</Button>
    </div>
  )
}

See the playground link.

So the issue seems to be associated with type inference when some of the type parameters are omitted?

@corbanbrook
Copy link

I have been playing around with building a polymorphic box component which takes native html props as well as atomic class names from vanilla-extract sprinkles.

  <Box display="flex" borderWidth="thick" />

Which can also use as:

  <Box as="input" type="email" /> 
  <Box as={Link} to="/home" /> // react-router-dom link

I came up against the problem of event handlers not typing the event properly and like others have noted here it is related to using types like Omit,Pick, ComponentPropsWithRef on your type. With some massaging I was able to get it to work.

You may have noticed that ComponentPropsWithoutRef seems to work fine, but ComponentPropsWithRef breaks when used on the top level of your type. So the below example uses and intersection of {ref?: Ref<ComponentPropsWithRef<T>['ref']> | null} to get around the issues mentioned above.

type BoxProps<T extends ElementType = 'div'> =
    ComponentPropsWithoutRef<T> & {
        as?: T
        ref?: Ref<ComponentPropsWithRef<T>['ref']> | null
    }

This example is largely based on the original types from the react-polymorphic-box lib.

playground link

@sscaff1
Copy link

sscaff1 commented Jan 12, 2023

@gynekolog I'm not omitting any props, but I do want to set a default for the ElementType and apparently that causes the issue again.

Here is your snippet with as an optional prop Link

Again it knows the type of the onClick, but it says the event is any. What's strange is that this is the same thing:

https://codesandbox.io/s/intelligent-marco-qdfwy4?file=/src/App.tsx

And yet codesandbox doesn't complain. When I try to do the same thing in a CRA project, I get the implicit any error as well.

@cristatus
Copy link

cristatus commented Jan 13, 2023

The issue is because of ElementType without any given parameter as it's type parameter defaults to any. See my last comment how I fixed it. May be you can infer the type parameter of ElementType to fix it.

@sscaff1
Copy link

sscaff1 commented Jan 13, 2023

I simplified your example and it still appears to work Link

What's strange is that if I paste my example, it also seems to work... I'm not sure what's going on.

@cristatus
Copy link

I simplified your example and it still appears to work Link

What's strange is that if I paste my example, it also seems to work... I'm not sure what's going on.

I see. May be it's fixed in some recent version.

@cristatus
Copy link

The type of as param in your original code should be inferred to make it work. You are passing the type directly.

@ayloncarrijo
Copy link

Seems like the problem occur just when using Omit or ComponentProps.

The reason why ComponentPropsWithoutRef works and ComponentProps doesn't is because ComponentPropsWithoutRef uses PropsWithoutRef, which uses a conditional type to omit the "ref" attribute. This gives us a workaround to fix the problem.

For now, using a conditional type on the omitted type seems to be enough to solve this:

Playground

import React from "react";

// -------------------- Without Omit (OK) --------------------

type PropsWithoutOmit<As extends React.ElementType> = {
  as?: As;
} & React.ComponentPropsWithoutRef<As>;

declare function TestWithoutOmit<As extends React.ElementType>(
  props: PropsWithoutOmit<As>
): null;

<TestWithoutOmit
  as="button"
  onClick={(value) => {
    // expected: typeof value = React.MouseEvent<HTMLButtonElement, MouseEvent>
    // actual: typeof value = React.MouseEvent<HTMLButtonElement, MouseEvent>
    // OK
  }}
/>;

// -------------------- With Omit (NOT OK) --------------------

type PropsWithOmit<As extends React.ElementType> = {
  as?: As;
} & Omit<React.ComponentPropsWithoutRef<As>, "a" | "b" | "c">;

declare function TestWithOmit<As extends React.ElementType>(
  props: PropsWithOmit<As>
): null;

<TestWithOmit
  as="button"
  onClick={(value) => {
    // expected: typeof value = React.MouseEvent<HTMLButtonElement, MouseEvent>
    // actual: typeof value = any (implicitly)
    // NOT OK
  }}
/>;

// -------------------- Omit Workaround (OK) --------------------

type Fix<T> = T extends any ? T : never;

type PropsWorkaround<As extends React.ElementType> = {
  as?: As;
} & Fix<Omit<React.ComponentPropsWithoutRef<As>, "a" | "b" | "c">>; // Fix here

declare function TestWorkaround<As extends React.ElementType>(
  props: PropsWorkaround<As>
): null;

<TestWorkaround
  as="button"
  onClick={(value) => {
    // expected: typeof value = React.MouseEvent<HTMLButtonElement, MouseEvent>
    // actual: typeof value = React.MouseEvent<HTMLButtonElement, MouseEvent>
    // OK
  }}
/>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.