Skip to content

Allow overloading of JSX element constructors #29025

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

Closed
5 tasks done
ferdaber opened this issue Dec 14, 2018 · 12 comments
Closed
5 tasks done

Allow overloading of JSX element constructors #29025

ferdaber opened this issue Dec 14, 2018 · 12 comments
Labels
Needs More Info The issue still hasn't been fully clarified

Comments

@ferdaber
Copy link

ferdaber commented Dec 14, 2018

Search Terms

jsx overload conditional component react

Suggestion

Allow for some kind of overload mechanism for JSX element construction, that is: determine contextual types for the allowable attributes for a JSX expression based on attributes already passed in.

Use Cases

There are times when we want to conditionally render based on the type of a certain property in a props object, or the existence/non-existence of a certain property in a props object. A very common use case is rendering clickable elements as anchor tags if a href attribute is present:

type AnchorProps = JSX.IntrinsicElements['a']
type ButtonProps = JSX.IntrinsicElements['button']
declare function Buttonish(props: ButtonProps): JSX.Element
declare function Buttonish(props: AnchorProps): JSX.Element
declare function Buttonish(props: AnchorProps | ButtonProps): JSX.Element

const myAnchor = <Buttonish href="typescriptlang.org" onClick={event => {}} /> 
// `event` is inferred to be React.MouseEvent<HTMLAnchorElement> because `href` is already there

The above is one suggestion though I don't know we could resolve that with class-based components.

I know right now that this is partially solved with generic components + conditional types, but it's kind of odd DX:

type EitherProps<T> = T extends string ? { href?: T } & AnchorProps : { href?: undefined } & ButtonProps
declare function Buttonish<T>(props: EitherProps<T>): JSX.Element

const myAnchor = <Buttonish href="typescriptlang.org">
// generic type inferred, but looks weird since T = string which doesn't tell you much

Examples

Unsure what the best approach would be. I think one good DX I was hoping for was for the autocomplete list to be narrowed as attributes are added to a JSX expression, when the compiler gets more contextual information to infer, so that after href is added in the above example, the event handlers are narrowed to just be handlers related to anchor elements, for example.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@weswigham
Copy link
Member

weswigham commented Dec 20, 2018

So this actually works as requested in 3.2. While some things broke in the great JSX migration (and are now fixed on @next), this is something that was fixed by it.
image

Are you using 3.2?

@weswigham weswigham added the Needs More Info The issue still hasn't been fully clarified label Dec 20, 2018
@ferdaber
Copy link
Author

I just tested it out and it works on my instance of 3.2, with a few interesting quirks (though they are all VSCode related but probably also related to the autocomplete list given by the compiler):

  • href is not in the list of attributes when you bring up the list while creating the JSX tag
  • The onClick handler attribute is still typed as the union of anchor and button mouse event handlers in the type popup in VSCode

Those are low priority though, and it looks like it works well with overloading the constructors on class components, so that's pretty awesome to see!

@ferdaber
Copy link
Author

Revisiting this, it looks like the overload signature works for function-type components, but is failing for class-type components, such as this example:

function isPropsForAnchorElement(props: ButtonProps | AnchorProps): props is AnchorProps {
  return 'href' in props
}

class Clickable extends React.Component<ButtonProps | AnchorProps> {
  constructor(props: ButtonProps)
  constructor(props: AnchorProps)
  constructor(props: ButtonProps | AnchorProps) {
    super(props)
  }

  render() {
    if (isPropsForAnchorElement(this.props)) {
      return <a { ...this.props } />
    } else {
      return <button { ...this.props } />
    }
  }
}

const aClickable = <Clickable href='abc' onClick={event => {}} /> // implicit 'any' on event
const btnClickable = <Clickable onClick={event => {}} /> // implicit 'any' on event

I'm assuming it's because it's using the JSX.ElementAttributesProperty functionality to identify the allowable attributes instead of the first parameter in the constructor call signature?

@weswigham
Copy link
Member

I'm assuming it's because it's using the JSX.ElementAttributesProperty functionality to identify the allowable attributes instead of the first parameter in the constructor call signature?

Yup. The irony is that if that jsx namespace member isn't present, we'll use the parameter type, just like functions nowadays.

@ferdaber
Copy link
Author

I wonder if it's better to just remove that specific interface in the JSX namespace then 🤔

I think this answers all my questions for now, thanks for the help!

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Oct 18, 2019

I just ran into this. @ferdaber What needs to happen so we can support overloading class components? I need this to define the correct types of react-autosuggest.

@ferdaber
Copy link
Author

Removing the ElementAttributesProperty interface declaration in the JSX namespace of React's type definitions should do the trick, like my above comment.

@OliverJAsh
Copy link
Contributor

Is this something we need to fix upstream in the React types, or are you suggesting doing this by patching the types locally?

@ferdaber
Copy link
Author

Either will work, but if you choose to patch it it does need to be a literal patch, since you can't remove declarations via declaration merging

@OliverJAsh
Copy link
Contributor

Thinking about this some more, why do you need overloads in your class example? Isn't the union parameter sufficient? Related: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#use-union-types

@ferdaber
Copy link
Author

Union parameter types are subtly different, and it may be a bug honestly (unsure). Consider the following:

type AElement = HTMLAnchorElement
type BElement = HTMLButtonElement

interface A {
    a: 'a'
    onClick(el: AElement): void
}

interface B {
    onClick(el: BElement): void
}

declare function AorB(props: A | B): JSX.Element
declare function AorBOverload(props: A): JSX.Element
declare function AorBOverload(props: B): JSX.Element

// TS error: `el` is inferred to be `any` even though it should be narrowed to `AElement`
const foo = <AorB a='a' onClick={el => {}} />

// this works fine and `el` is inferred to be `AElement`
const bar = <AorBOverload a='a' onClick={el => {}} />

@OliverJAsh
Copy link
Contributor

@ferdaber Interesting, thanks. Opened an issue: #34590.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs More Info The issue still hasn't been fully clarified
Projects
None yet
Development

No branches or pull requests

3 participants