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

Type JSX elements based on createElement function #14729

Open
ivogabe opened this issue Mar 19, 2017 · 44 comments · Fixed by #51328
Open

Type JSX elements based on createElement function #14729

ivogabe opened this issue Mar 19, 2017 · 44 comments · Fixed by #51328
Assignees
Labels
Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@ivogabe
Copy link
Contributor

ivogabe commented Mar 19, 2017

TypeScript currently uses a JSX namespace to type the props of JSX elements. This is customisable by modifying JSX.IntrinsicElements, JSX.ElementClass. However, the type of an element is always JSX.Element.

Of course more interfaces could be added to the JSX namespace. However, I think that with less effort, the compiler could be made more flexible. I'd propose to check JSX elements based on a (virtual) call to JSX.createElement. For instance, the current behaviour can approximately be written like this:

namespace JSX {
  // intrinsic elements
  function createElement<K extends keyof JSX.IntrinsicElements>(tag: K, props: JSX.IntrinsicElements[K], children: JSX.Element[]): JSX.Element;
  // class and functional components
  function createElement<P>(component: (JSX.ElementClass & { new(): { props: P } }) | ((props: P) => JSX.Element), props: P, children: JSX.Element[]): JSX.Element;
}

Given that function signatures are well customisable with the use of generics for instance, most requests can be implemented this way. For instance, #13890 and #13618 would benefit from this change. Libraries that use JSX, but not on the React-way, will benefit from this too.

Proposed semantics

For a JSX element, a virtual call to JSX.createElement is constructed. It is passed three arguments:

  1. Tag name (string) in case of an intrinsic argument (<div />). Otherwise, the identifier is passed (Foo for <Foo />).
  2. An object containing the props.
  3. An array containing the children.

This should be roughly the same as the JSX transform used in the emitter phase. One notable difference is the following: in case of no properties or no children, an empty object or an empty array should be passed, instead ignoring the argument. This makes it easier to write JSX.createElement for library authors.

Backwards compatibility

For backwards compatibility, one of the following approaches could be taken:

  • Use old logic if JSX.createElement does not exist.
  • If JSX.createElement does not exist, default it to (roughly) the definition above.

Error reporting

When type checking the generated function call, the checker can give error messages like "Argument of type .. " or "Supplied parameters do not match any signature of call target". These messages should be replaced when checking JSX elements.

@RyanCavanaugh
Copy link
Member

I think this is doable and could potentially cut out a lot of code from the compiler. When we added JSX support we didn't have spread types or keyof / T[k] so this was impossible. Behavior-wise I think this approach would allow all current behavior to be represented with the exception of data- and aria-, which would could special case to not present excess property errors.

The remaining difficulty is just technical - the overload resolution code is hardcoded in a lot of places with the assumption that it's operating over an actual argument list syntax tree rather than some abstract set of parameters.

@niieani
Copy link

niieani commented Mar 22, 2017

Having this would allow one to utilize the concept of generalized JSX with TS. Currently it seems impossible, because the return type of any JSX expression is always a non-generic JSX.Element.

In fact, given this it would make sense to ditch the whole "JSX" global namespace and just use the declared type of the locally scoped jsxFactory (set in the tsconfig.json), so that the return type of a JSX expression could be different in every file, based on the implementation of the jsxFactory.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this In Discussion Not yet reached consensus Suggestion An idea for TypeScript and removed Help Wanted You can do this labels Mar 22, 2017
@ivogabe
Copy link
Contributor Author

ivogabe commented Apr 4, 2017

I think this is doable and could potentially cut out a lot of code from the compiler. When we added JSX support we didn't have spread types or keyof / T[k] so this was impossible.

That's exactly what I thought!

The remaining difficulty is just technical - the overload resolution code is hardcoded in a lot of places with the assumption that it's operating over an actual argument list syntax tree rather than some abstract set of parameters.

The compiler already handles different kinds of function calls. resolveSignature in checker.ts works with normal call expressions, but also tagged templates and decorators. Those last two don't have an explicit argument list in the source code, so I think that JSX components could be integrated there too? Actually, that function already handles JSX elements for the language service, when used with SFCs.

@niieani That's an interesting use case. I think that it would be useful in a lot of situations, other than the DOM, where some domain specific language is desired.

@niieani
Copy link

niieani commented Apr 6, 2017

@ivogabe yeah, I especially like the example with ASTs, represented by JSX. So much more readable than declaring plain objects for AST.

@ceymard
Copy link

ceymard commented May 15, 2017

I'd like to ping this issue ; could it be possible that the whole jsx mechanic be simply a transform to a function call with its implied checking ?

This would allow for so much flexibility in library creation.

@RyanCavanaugh
Copy link
Member

could it be possible that the whole jsx mechanic be simply a transform to a function call with its implied checking ?

No. Two reasons and there may be more:

  • In React properties like data-foo and aria-bar are special, but we'd incorrectly flag these as errors
  • We wouldn't detect excess properties if there was a spread [1]

[1]

interface Point {
    x: number;
    y: number;
}

var m: Point;
var j: Point = { z: 10, ...m }; // Not an error due to spread

@niieani
Copy link

niieani commented May 16, 2017

@RyanCavanaugh The first could be fixed via other types of constructs that could be added to the language:

WithDomProperties<Point> -- adds typing data-* and aria-* to the type

Second we could think about. Maybe the spreads in JSX would behave differently?

There's probably something we could do. In any case, this would massively simplify the API and allow for custom JSX applications as I've mentioned in the comment above.

@amir-arad
Copy link

AFAIK it's not just data-* and area-*, it's *-*.

class Foo extends React.Component<{a:string}>{}

//This works:
<Foo b-3="bar" a="ggg"/>

@joewood
Copy link

joewood commented Dec 12, 2017

@RyanCavanaugh now that the second point has been fixed, can this be revisited?

interface Point {
    x: number;
    y: number;
}

var m: Point;
var j: Point = { z: 10, ...m }; // NOW ERRORS

@jchitel
Copy link

jchitel commented Feb 6, 2018

Another problem to consider here is how to deal with backwards compatibility, particularly around the DefinitelyTyped definitions for React, which would have to be updated to support this.

Existing types that depend on the JSX namespace would no longer work because the compiler wouldn't look there anymore. The most graceful way I can see to handle that would be to modify the React definitions so that the JSX namespace definitions are dependent on (or at least structurally equivalent to) whatever types are used for React.createElement, and deprecate the JSX namespace.

For non-React use cases, this will probably just have to be a breaking change, with a suggested migration pattern. If someone is already using the JSX namespace for typing JSX, all they need to do is add a jsxFactory function (which they probably already have) and tie their JSX types to it.

I agree that having this would improve the flexibility of JSX-in-TS considerably, but the migration for existing code will probably be quite painful.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 13, 2018

should be covered by #21699

@reverofevil
Copy link

reverofevil commented Feb 14, 2023

@eps1lon That's because TS has a buggy intentionally different implementation of unification: it generates an unknown whenever you ask to unify monotype to polytype. Verbatim translation of your code to any language that implements it correctly works.

data Elem = Elem String {- | ... -} deriving (Eq, Show)

data Props t = Props { items :: [t], render :: t -> Elem }

component :: Props t -> Elem
component _ = Elem ""

type FC p = p -> Elem

jsx :: FC p -> p -> Elem
jsx f p = f p

main = print $ jsx component (Props ["one", "two"] (\item -> Elem (show (length item))))

Playground link

@TylorS
Copy link

TylorS commented Mar 2, 2023

I've been following various JSX threads over the years all boiling down to this, I really truly hope this lands in TS at some point even if not 5.1 directly.

As you fear the performance of JSX return types, what about a compromise? Either:

  • enable with an option that defaults to disabled?
  • allow return types only for top level result of a JSX expression so at least something like const div =
    ...
    regardless of what children the div has.

In response to this question, I sincerely hope for it to be 1. The type-safety of these functions seems most important over performance issues. The Effect community (formerly fp-ts) and I are building libraries today for html rendering and are today forced to use tagged template literals to be able to track typed resources and typed errors of children to be able to aggregate the resources required to run them and the errors they might produce as unions.

If JSX uses createElement as any other function for type-checking, we'd be able to utilize JSX's AST to further optimize them using compiler techniques and provide much more type-safe APIs for properties which is a lot more cumbersome to do with template literal strings and requires language service/server plugins to work as expected.

@TylorS
Copy link

TylorS commented Apr 14, 2023

@RyanCavanaugh You closed this with #51328, but it doesn't at all lookup the types by the signature of createElement. I think this should remain open

@yairopro
Copy link

yairopro commented Jun 14, 2023

I just add here a pure sample code of the problem:

https://codesandbox.io/s/jsx-element-loses-types-6nmh3m?file=/src/App.tsx

@ghost
Copy link

ghost commented Jul 17, 2023

Maybe there's something I don't get here. Why is it not possible to allow JSX.Element to be generic? Simply passing it the JSX type (the literal string in the case of intrinsic elements and the type of the symbol in the case of value-based elements) would solve a lot of issues). Something like the following could be done:

  • Pass the JSX type as the first type parameter to JSX.Element for flexibility.
    declare namespace JSX {
      interface Element<JSXElementType> {
        // Definitions can go here
      }
    }
  • For every *.tsx file, enforce that the type of the resolved jsxFactory or the export named jsx in the jsxImportSource (whichever one is in scope) equals or extends the following JSXFactory:
    type GlobalJSXElementType = JSX.ElementType
    
    type JSXElementTypeProps<JSXElementType extends GlobalJSXElementType> =
      // If the jsx element is an intrinsic element
      JSXElementType extends keyof JSX.IntrinsicElements
        ? JSX.IntrinsicElements[JSXElementType]
    
        : // Else if the jsx element is a function component
        JSXElementType extends (props: infer FCPropTypes) => any
        ? FCPropTypes
    
        : // Else if it is a class component with attributes property defined
        JSXElementType extends new (props: any) => {
            [Key in keyof JSX.ElementAttributesProperty]: infer CCPropTypes
          }
        ? CCPropTypes
    
        : // Else the prop types of the class component should be taken from its constructor
        JSXElementType extends new (props: infer ContructorPropTypes) => object
        ? ContructorPropTypes
        : never
    
    type JSXFactory = <JSXElementType extends GlobalJSXElementType>(
      jsxElementType: JSXElementType,
      props: JSXElementTypeProps<JSXElementType>,
      key: string
    ) => JSX.Element<JSXElementType>

I'm new here, so maybe I'm missing something. I do know that this approach might have some performance issues similar to the one discussed here.

Also, does Typescript always pass the key as the third parameter to the factory function when compiled? I'm not sure.

@ghost
Copy link

ghost commented Jul 17, 2023

It could be an opt-in feature behind a TSConfig option since it'll break existing code.

@ghost
Copy link

ghost commented Jul 18, 2023

So, @MartinJohns and @xxaff, do you think it's a bad idea? I'll like to learn. I'm new to the TS world.

@arthurfiorette
Copy link

This was planned for milestone 5.1. Any updates here?

@frzi
Copy link

frzi commented Apr 25, 2024

Is there still any interest or plans for this? This seems like an incredible feature that’ll make libraries utilizing JSX far more versatile. 😄

@robbiespeed
Copy link

Here's a couple TS Playground examples of how we could use this feature to enable similar features to flow's "renders" types and limiting allowed children or other props that receive jsx:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet