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

Decouple jsx element type from jsx factory return type and sfc return type #21699

Open
weswigham opened this issue Feb 6, 2018 · 115 comments · Fixed by #51328
Open

Decouple jsx element type from jsx factory return type and sfc return type #21699

weswigham opened this issue Feb 6, 2018 · 115 comments · Fixed by #51328
Labels
Breaking Change Would introduce errors in existing code Domain: JSX/TSX Relates to the JSX parser and emitter Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Milestone

Comments

@weswigham
Copy link
Member

weswigham commented Feb 6, 2018

We need to look up the type of a jsx expression by actually resolving the jsx factory call, so that we don't create a reference to the global JSX.Element type, which can change shape between react versions (as it needs to in the react 16 upgrade). We also need to resolve the sfc return type and class element type from the parameters of the factory function overloads for the same reasons, doubly so because the types allowable as render method and SFC return values are no longer the same as JSX.Element (namely, they can be strings, arrays, portals, etc).

This might be considered a breaking change, because some consumers may expect JSX.Element to always be a supertype of both jsx element expression return types and SFC return types (even though this isn't true in react 16) - we certainly made that assumption internally, hence the need for the change. 🐱

@aaronjensen
Copy link

@weswigham would addressing this address #18357 ? It'd be great to be able to correctly type children render props in React.

@jchitel
Copy link

jchitel commented Feb 14, 2018

There are a lot of issues related to this that have been closed, saying that this issue will handle them. I am assuming that the case in #18357 will be handled by this as well.

At the core of this is the ability to type JSX based off of the createElement function, rather than using the set of interfaces in the JSX namespace. If that feature is indeed going to be implemented as part of this issue, then a natural extension would be that the children could end up anywhere in the resulting element, and they could be typed based off of anything, rather than just restricting children to the props.

@ericanderson
Copy link
Contributor

@jchitel agreed. I just hit this snag trying to change the react typings yesterday for this exact same reason.

@ericanderson
Copy link
Contributor

@weswigham If you haven't started on this yet, I'd like to take a crack at it

@weswigham
Copy link
Member Author

@ericanderson I actually started work on this today, sorry 😉

@ericanderson
Copy link
Contributor

@weswigham Damn. Can you make sure I can capture the SFC or ComponentClass so we can do things like limit the children of <ButtonGroup> to <Button>?

@weswigham
Copy link
Member Author

weswigham commented Feb 22, 2018

@ericanderson That's the plan. (I mean, technically it was a bug report a long time ago that we closed as "fixed" even though it was only half-fixed)

@ericanderson
Copy link
Contributor

I think that will also let us mark the defaultProps optional.

@weswigham
Copy link
Member Author

By defaultProps do you mean the intrinsic props?

@ericanderson
Copy link
Contributor

ericanderson commented Feb 22, 2018

I mean:

interface Props {
  foo: string,
  bar: number
}
class Foo extends React.Component<Props> {
  static defaultProps = { foo: "Hi Mom!" };
//...
}

In this world, the compiler should only require me to specify bar and foo should now be considered foo?: string

@weswigham
Copy link
Member Author

... maybe. Even with localized types looked up from the signatuers, we still have to make assumptions about how the custom ctor/sfc is effectively built, because there's actually two calls going on. The first call is the constructor for the class or the SFC, (which are expected to take props as an argument), and can be overloaded - this is where a lot of props validation actually happens. The second call is the factory function itself, which actually needs the reified types from the inner call to be typechecked correctly (And today isn't actually typechecked at all, which is the root of most of these issues).

TBQH a lot of the JSX machinery in-place today is to get higher-ordery behavior from a system that used to not support any. I think now that we have conditional and infer types, there's a good chance I may actually be able to desugar a lot of the magic currently applied to JSX to normal typesystem operations. I'm generally just going to look at improving this as much as is feasible 😉

@ericanderson
Copy link
Contributor

So the props we should enforce are actually Omit<Props, typeof Foo.defaultProps> & Partial<typeof Foo.defaultProps>... which maybe we can't do with this change.

@ericanderson
Copy link
Contributor

@weswigham Thanks!

@DanielRosenwasser
Copy link
Member

I'm not entirely sure that this was technically fixed. #18131 was fixed, but we still don't actually look at what a given JSX factory invocation returns.

@dead-claudia
Copy link

dead-claudia commented Jul 16, 2024

@bluepnume SolidJS doesn't compile JSX to simple function calls. They currently compile them to IIFEs for the DOM, and a _$ssr(template, ...) call (almost a template tag but not quite) for server-side rendering. Edit: And the return types for server and client rendering are very different, with it returning an element when compiled for client-side rendering and a {t: string} object when compiled for SSR. The precise type I believe is actually an implementation detail and should be an opaque type.

@bluepnume
Copy link

bluepnume commented Jul 16, 2024

Sure, but I don't think it's that outlandish for typescript to default to simple function calls given that that is already how the tsc transpiler handles them. For example this is the tsc output:

/** @jsx jsxFactory */

const foo = ( <div /> )
"use strict";
/** @jsx jsxFactory */
const foo = (jsxFactory("div", null));

-- it feels like this should be the default for the type system too, with some way to override for frameworks like SolidJS that don't follow this convention?

@anilanar
Copy link
Contributor

anilanar commented Jul 16, 2024

Yes, that's called a syntactic sugar and I think that's the most future proof way to do it in my opinion but that may not be the most practical one considering the kind of baggage TS carries due to backward compatibility in terms of both functionality and performance.

If a target library (like SolidJS) needs to use a black box, they are free to do so.

@dead-claudia
Copy link

@bluepnume TS also supports outputting JSX literally, mainly for React Native, but Solid requires this as well.

@thesoftwarephilosopher
Copy link

thesoftwarephilosopher commented Aug 19, 2024

Is this the issue that tracks the ability to make this finally happen?

declare namespace JSX {

  type IntrinsicElementTypes = {
    button: HTMLButtonElement,
    a: HTMLAnchorElement,
    ul: HTMLUListElement,
    '': DocumentFragment,
  }
}

const b = <button />; // now inferred as HTMLButtonElement
const a = <a />;      // now inferred as HTMLAnchorElement
const f = <Foo />;    // now inferred as HTMLUListElement
const d = <></>;      // now inferred as DocumentFragment

function Foo() /* return type inferred as HTMLUListElement */ {
  return <Bar />;
}

function Bar() /* return type inferred as HTMLUListElement */ {
  return <ul />;
}

Because I really need that feature since I use vanillajsx daily.

I'd be glad to contribute towards this feature full time until it's done.

Also, IntrinsicElementTypes would be backwards-compatible, as its default definition would be:

type IntrinsicElementTypes = {
  [tag: string]: JSX.Element,
  '': JSX.Fragment,
}

@reverofevil
Copy link

reverofevil commented Aug 19, 2024

@sdegutis Yes, this is the issue. No, correct implementation breaks TS type checker due to a classic unfixable mistake in language design. Here are similar issues in Swift and C# (yes, Anders did it twice).

@dyst5422
Copy link

@sdegutis Yes, this is the issue. No, correct implementation breaks TS type checker due to a classic unfixable mistake in language design. Here are similar issues in Swift and C# (yes, Anders did it twice).

This should be pinned to the top of this thread to prevent others from wasting their time here.

@thesoftwarephilosopher
Copy link

@sdegutis Yes, this is the issue. No, correct implementation breaks TS type checker due to a classic unfixable mistake in language design. Here are similar issues in Swift and C# (yes, Anders did it twice).

This should be pinned to the top of this thread to prevent others from wasting their time here.

Assuming @reverofevil is correct. He's certainly confident, but I've met too many overconfident software engineers who turned out to be wrong. I'd need to see some kind of proof, or someone with reknowned credibility confirming this feature is "unfixable".

@cowboyd
Copy link

cowboyd commented Aug 22, 2024

@sdegutis I came to this issue for a very similar reason that you did. I just wanted jsx to function as an alternate syntax for creating JavaScript values.

You may want to check out hastx It seems like it is very similar in spirit to vanillajsx. It just creates a value that represents an HTML AST. The main reason it exists is to be an extension to the UnifiedJS ecosystem which includes things like MDX and friends.

@reverofevil
Copy link

reverofevil commented Aug 22, 2024

@sdegutis Oh, I'd be very happy to be proven wrong! But just for the context:

TS has something called "context-sensitive resolution", the thing that allows you omit function argument types when that function is assigned into a typed variable

type F = (x: number) => void
const f: F = x => console.log(x + 1); // x: number

Because of this, for code with two nested calls to overloaded functions, TS has to first iterate over overloads of the outer function, so that expected argument type can be applied as context to resolve inner function

interface F {
  (x: true): number;
  (x: false): string;
  (x: boolean): number | string;
}
interface G {
  (f: (x: 1) => void): true;
  (f: (x: 2) => void): false;
  (f: (x: 1 | 2) => void): boolean;
}
declare const f: F;
declare const g: G;
const r: number = f(g(x => console.log(x))); // x: 1

Here TS should resolve the type for f in context of number, iterates over its overloads, and for each of those overloads does another context-sensitive resolution for g with 2 overloads. So we got 2^2 = 4 function signature checking operations.

In case of JSX there a thousands of overloads in createElement, and tens of nested calls, which results in thousands^tens ~ 10^30 function signature checking operations per component. I'm not an expert in TS codebase, and didn't research it properly, but take a look at the comment of TS developer who did.

Performance is terrible. Like beyond bad. This makes every jsx tag tree in your program into a series of nested generic context sensitive function calls (and most jsx apps have a lot of nested tags!). That's just about the worst-case scenario for the type checker.

I don't see any solutions here except for "do not check types in a way TS does it". Textbook implementation would use (linear-time) unification instead of context-dependent resolution, and would constrain ad-hoc polymorphism so that every signature is an instance of a more generic type to check call sites against. Unfortunately, first thing is incompatible with pretty much all novel types added to TS (especially conditional types), and second would require changing all the syntax and semantics of overloading.

But again, who am I to tell about type theory textbooks. I'm pretty sure developers knew what they're doing, and made intentionally exponential type checker, so that we have time for a coffee while the code compiles.

@thesoftwarephilosopher
Copy link

@sdegutis I came to this issue for a very similar reason that you did. I just wanted jsx to function as an alternate syntax for creating JavaScript values.

You may want to check out hastx It seems like it is very similar in spirit to vanillajsx. It just creates a value that represents an HTML AST. The main reason it exists is to be an extension to the UnifiedJS ecosystem which includes things like MDX and friends.

@cowboyd Yeah it'd be great if JSX expressions could be typed based on usage, it would be a win for the whole community, if only this other guy who thinks he knows more than Anders turns out to be wrong, or if he's right but tsc deprecates function overloading (probably unlikely tho I'd be okay with it personally since I never use it and consider it bad practice anyway).

Funny enough, I think my original JSX Monarch that I've since lost the code to and forgot how to do was actually written as part of a proprietary closed-source alternative and/or extension to MDX for a client a few years ago. It's interesting to see how the community keeps dividing with oh so similar ideas that are almost compatible. I wish I could use hastx in vanillajsx to solve that. But then I'd still need a compiler, which defeats the "vanilla" in vanillajsx, or I'd need to add a helper function that transforms the AST to DOM objects, which is what I originally had in imlib before I landed on what I have now. (Technically I'm already using a compiler, but that's only because there's no vanilla JSX in ES spec yet. Ideally JSX would be native syntactic sugar for something, and I've been brainstorming what that might be for a few years, but I'm not really part of any community, so it's effectively just me wondering out loud, which is pointless and ineffective.)

@cowboyd
Copy link

cowboyd commented Aug 22, 2024

Hmm..... reading over your thoughts, it sounds like what is needed is jsx literals

The closest thing I can find is this discussion which seems to have petered out a couple of years ago. Just glancing over it though, it seems like it's too complicated. What would be wrong with a straightforward literal syntax for javascript values?

<a href="foo">bar</a> // =>  { tag: "a", attrs: { href: "foo" }, content: "bar" }

"Components" are not special, they are just a function in tag position.

const Foo = () => {}
<Foo x=1 y=0/> // => { tag: Foo, attrs: { x: 1, y: 0 } }

Because there is no factory function at all, there are no magical overloads, the structure of the value is known at compile-time and so any functions accepting or returning said values can be typed according to existing rules.

@thesoftwarephilosopher
Copy link

@cowboyd that would be the ideal way to make JSX vanilla in ECMAScript and provide automatic typing for it in TypeScript. A couple years ago I experimented with it briefly and found it was a little too slow to my use case, though I admit I didn't try very well to see what I could do to optimize it. There's also the difficulty that it would still need a helper to transform it to DOM nodes or react elements or whatever else, but that's a small price to pay for vanilla JSX support, and compilers could still exist that pretransform it to what we need for performance so that it never becomes vanilla JSX in the first place. So I'm leaning towards proposing that. Though I'd still like to see what could be done to make it configurable via vanilla. Hence that issue I made in vanillajsx repo.

@cowboyd
Copy link

cowboyd commented Aug 22, 2024

There's also the difficulty that it would still need a helper to transform it to DOM nodes or react elements or whatever else,

@sdegutis That's a feature not a bug, right? It's just a function that takes a value and returns a value. These helper functions already exist today, they just require a variadic pile of magical hacks to the runtime and the type system in order to work. React would still have render() it just wouldn't be anything special.

createRoot(document.documentElement).render(<body/>);

This would happily exist alongside a rehype transformation in the same file

const modded = rehypeAutoLinkHeadings()(
  <section>
    <h1>Chapter 1</h1>
    <p>It was the best of times. It was the worst of times...</p>
  </section>,
);

And indeed could be freely mixed:

createRoot(document.documentElement).render(<body>
  {rehypeAutoLinkHeadings()(
    <section>
      <h1>Chapter 1</h1>
      <p>It was the best of times. It was the worst of times...</p>
    </section>,
  )}
</body>);

@thesoftwarephilosopher
Copy link

thesoftwarephilosopher commented Aug 22, 2024

@cowboyd I like it. Seems like the future of JS to me. I'd recommend we talk to the various JSX communities and see if this idea can gain traction. But since that's off topic, I offer thesoftwarephilosopher/imlib#5 as a place for discussing that goal further. But on the topic of what TypeScript might type JSX expressions as in the future, one thing I would recommend is that JSX be transpiled to

type JSX = { jsx: any, children: any, [attr: string]: any };

<a href="foo">bar</a> as { jsx: "a", href: "foo", children: ["bar"] }
<Foo x='1' y={0} />   as { jsx: Foo, x: '1', y: 0 }

It's shorter, it uses fewer objects, and it can be conventionally inferred as coming from JSX (or mocked to look like it did).

@frzi
Copy link

frzi commented Aug 22, 2024

**sdegutis ** commented Aug 22, 2024

This is redefining pretty much everything about JSX as we know it. Which is 1) out of scope and off topic, and 2) not the topic of this issue.

The JSX syntax and what it transpiles to is fine. There are zero issues with this. The issue is Typescript failing to properly type check the JSX syntax based on the factory function and instead requires a whole song and dance with JSX-namespaced interfaces. 🙂

To illustrate the problem with just a few lines of code, copy the following snippet into your favorite IDE that supports Typescript:

/** @jsx h */

function h<T extends keyof HTMLElementTagNameMap>(
	tag: T,
	props: Partial<HTMLElementTagNameMap[T]>,
	...children: (string | HTMLElement)[]
): HTMLElementTagNameMap[T] {
	// Pretend we care about `props` and `children`.
	return document.createElement(tag)
}

const div = <div id='foo'>bar</div>
const div2 = h('div', { id: 'foo' }, 'bar')

(Or check it out in the official playground)

All kinds of things go awry with const div.

  • It's complaining about a JSX.IntrinsicElements
  • Its type falls back to any
  • No proper type checking on the tagname
  • No proper type checking on the attributes
  • No proper type checking on the children

But look at the code it compiles to. It's exactly the same as const div2. And if you play around with the factory function (h()) directly you'll see that:

  • It returns the proper type (in this case HTMLDivElement)
  • It type checks the tagname (and provides autocomplete)
  • It type checks the attributes (and provides autocomplete)
  • It type checks the children (try putting a number or a boolean as children)

This issue requests for the JSX syntax to be provided with exactly the same type safety and features as you get when using the factory function directly. This will greatly simplify the development of frameworks that utilize JSX; will actually make the JSX syntax more powerful and flexible; and will make using multiple JSX powered libraries less of a pain as there'd be no conflicts or clashing over the JSX namespace (in fact, the whole JSX namespace and all its interfaces will become redundant).

Now admittedly I'm not intimate with the inner workings of the Typescript type checker and compiler. And I'm sure the whole JSX-namespace is deeply rooted into the code. But I find it a bit too defeatist to simply shrug our shoulders and claim Typescript has gone too far to ever be able to support this improvement. 😞

@bluepnume
Copy link

@reverofevil what you're saying makes sense; but in that case wouldn't FlowType have exactly the same problem? Because in Flow, inferring jsx types from the factory function works perfectly.

@thesoftwarephilosopher
Copy link

The JSX syntax and what it transpiles to is fine. There are zero issues with this. The issue is Typescript failing to properly type check the JSX syntax based on the factory function and instead requires a whole song and dance with JSX-namespaced interfaces. 🙂

(a) Not everyone agrees that JSX is currently fine with no issues, and (b) if it changed to what's being proposed, everything you described wouldn't be an issue anymore, and typing it would become significantly easier, which is why it's I think still on topic.

@bluepnume
Copy link

bluepnume commented Aug 22, 2024

I assume there's a reason the original React/jsx authors didn't settle on this object structure -- since it feels a lot more natural than the current factory functions. Perhaps because it's a bit more wasteful to construct an intermediary throwaway object versus directly creating the virtual dom in the factory function in the optimal structure?

@thesoftwarephilosopher
Copy link

thesoftwarephilosopher commented Aug 22, 2024

@bluepnume Pretty sure that's why Facebook compiled JSX directly to React.createElement, and maybe at the time it had merit, but there's no point in protecting React from becoming slow at this point. And if the whole community is going to benefit from standardized JSX, it's going to have to come with the cost of being generic, even if that means intermediate object creation is less than ideal.

@cowboyd
Copy link

cowboyd commented Aug 22, 2024

I assume there's a reason the original React/jsx authors didn't settle on this object structure -- since it feels a lot more natural than the current factory functions. Perhaps because it's a bit more wasteful to construct an intermediary throwaway object versus directly creating the virtual dom in the factory function in the optimal structure?

@bluepnume @sdegutis Or they just didn't have a bajillion use-cases and a decade of hindsight with which to work ;-)

Ironically, it was its counterintuitive (for the time) intermediate representation and the corresponding power to reason about the DOM tree as a value that made React fast in the first place.

@nhh
Copy link

nhh commented Aug 29, 2024

Just an idea, why dont we configure our bundlers/tsc to a two step compilation.

  1. Transpile jsx => ts (instead of js)
  2. Inject proper jsx factory functions
  3. Compile everything to js

__jsx("div") => mapped to a intrinsic element with "div" as key.

EDIT: DX would be aweful :/

@dead-claudia
Copy link

I have an idea: what about an alternate JSX checker mode that directly uses a named JSX factory instead of indirectly just using types?

The idea is that, in this mode, assuming the JSX factory is h, <Foo id="bar"><Bar /><Foo> would be desugared to h(Foo, {id: "bar"}, h(Bar, null)) during parsing. A flag on call nodes could be added to guide error messages, but this would make it much simpler of an issue to address and wouldn't require substantial type system changes.

Such a flag, when active, will slow compilation down somewhat and result in higher memory usage, but 1. it'd grant the flexibility everyone needs and 2. you could cache most of that memory overhead away.

@thesoftwarephilosopher
Copy link

I just published a proposal to standardize JSX.

@dead-claudia
Copy link

@sdegutis Discussion around ECMAScript language proposals is better suited for https://es.discourse.group/. This issue is strictly about TypeScript's type checking of the JSX language extension as currently used by various frameworks and libraries.

@nhh
Copy link

nhh commented Sep 4, 2024

In regards to add proper children types, I think this already works. In dojo/framework#43 is mentioned, that the ElementChildrenAttribute and the ElementAttributesProperty are staying in relation.

So I am able to type the children structure:

declare namespace JSX {
  // Define that only my classes are valid jsx
  type ElementType = typeof Hello | typeof World | string

  // Define the props, and the props.children attribute
  type ElementChildrenAttribute = { children: {} }
  type ElementAttributesProperty  = { props: {} }
  
  // Make sure no one uses lowercase directives:
  type IntrinsicElements = never
}

declare class Hello {
  props?: {
    foo?: string;
    children?: World[]
  }
}

declare class World {
  props?: {
    foo?: string;
    children?: any
  }
}

export default () => (
  <Hello>
      <World>{"This is valid, because the children of World are any"}</World>
      {"This will not compile, as the children of Hello must be a list of World class instances but this is a string"}
  </Hello>
);

EDIT: This is close, but it still does not let me properly define siblings as children
EDITEDIT: Seems to have something todo with the root Element getting children: any[] as type params
EEE: Nevermind this does not work due to se same limitations normal directives have

Typescripts JSX expressions are always any or JSX.Element as stated here: https://www.typescriptlang.org/docs/handbook/jsx.html#the-jsx-result-type

Type '{ children: [Element, Element, Element, Element, string, number]; }' This is the type that the compiler is infering, funnily it gets string and number correct.

export default () => (
 <Foo>
   <Foo>
     <Foo></Foo>
   </Foo>
   <Foo></Foo>
   <Foo></Foo>
   <Foo></Foo>

   {"asdasda"}
   {2}
 </Chart>
);

Even more funny is the fact, that the compiler is able to infer the correct type when using type hints like this

Type '{ children: [Element, Element, Element, Element, string, Foo]; }'

{2 as Foo}

@yohan-pg
Copy link

@reverofevil I'm not sure I understand the problem with context-sensitive resolution. Shouldn't it be possible to compile to a function without overloads like h.div(...) instead of h("div", ...) and avoid all of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Domain: JSX/TSX Relates to the JSX parser and emitter Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet