Return type for fragments (`<>...</>`) with JSX automatic import

[x11x]

I may have misconfigured something (my jsx runtime settings or tsconfig) or misunderstood how to use the API, but it seems that JSX fragments (<>...</>) are typed as React.ReactElement, which seems to be defined here as (HTMLElement | SVGElement) :https://github.com/alex-kinokon/jsx-dom/blob/a5682a5b89696726c7461e9398ada6091b9d518d/types/index.d.ts#L61

But at runtime they are DocumentFragment (which is a subclass of Node and not Element or HTMLElement/SVGElement).

This issue might be caused by or at least related to https://github.com/Microsoft/TypeScript/issues/21699, (which you have mentioned in the readme), but I wasn't sure. If this is the case, please disregard this issue.

My config

I am using the React 17+ style "automatic import", with the following in my tsconfig.json:

{
    "jsx": "react-jsx",
    "jsxImportSource": "jsx-dom/min",
}

Maybe I need to use the older jsxFactory/jsxFragmentFactory instead of the newer automatic JSX runtime import? But I think these control the emitted code, not necessarily the type-checking behavior. Looking through the TypeScript docs I have not been able to find anything about setting the type for fragments. I thought there might be a way to configure it within the declare namespace JSX { ... } but have not found a way.

Not sure if this is relevant because my issue is with type-checking not runtime, but I am using typescript with noEmit, and esbuild as the bundler, which finds its configuration in the tsconfig.json "jsx" and "jsxImportSource" and all seems to work correctly.

Runtime behavior (works as expected)

Despite the incorrect typings, fragments actually do work at runtime because from what I can tell, this line detects a DocumentFragment as an Element and then appends it (which works fine with DocumentFragments just like most other Node subclasses except Document I guess). https://github.com/alex-kinokon/jsx-dom/blob/a5682a5b89696726c7461e9398ada6091b9d518d/src/jsx-dom.ts#L220-L221 This is due to the definition of isElement which in terms of the DOM class hierarchy, actually checks that the argument is a Node (i.e. superclass of Element). https://github.com/alex-kinokon/jsx-dom/blob/a5682a5b89696726c7461e9398ada6091b9d518d/src/util.ts#L13-L15 Technically it checks that the argument is of type {nodeType: number}, to which Node is assignable, along with all its subclasses including Element and DocumentFragment, so I understand why this code was written like this. But it allows DocumentFragments to slip through as Elements when the JSX fragment syntax is used.

(As a side issue maybe this function should be changed to function isNode(val: any): val is Node to avoid future bugs/confusion? but this is not an issue for me as it seems to be a private API, and works fine for its intended use, so I'm not complaining about this, just pointing this out).

Summary

So I'm just wondering if anyone knows how to correctly configure this on my end, or has any thoughts about strategies/techniques for ways to change jsx-dom to better support fragments with correct TypeScript types, particularly with the new-style automatic JSX runtime.

And also whether it would be worth changing the type of JSX.Element or ReactElement to include DocumentFragment? -- but this could potentially cause a bunch more issues, require changing some other parts of the jsx-dom API, and could be a big breaking change, so I'm not sure if this is the way to go.

Having components (functional or class-based) return a DocumentFragment at runtime, but with the type-checker thinking its a HTMLElement | SVGElement works in many simple cases such as appendChild but could obviously lead to bugs/inconsistencies - and also confuse developers if they don't dig into the jsx-dom code and figure out what is actually returned when they use JSX fragments. But in any case, the JSX literals are already a bit of a black box due to the aforementioned TypeScript issue, so I guess to be robust, it is worth checking the type of the JSX literals is what you expect at runtime and using type guards/casts as needed.

Its possible this has already been considered/discussed and the current behavior chosen as the best compromise, which would be fine because as I said, it works at runtime.

Thanks for this great library.

[alex-kinokon]

Thanks for the write up. Can you give me an expected/actual case with a type error for testing?

[x11x]

Hi, The simplest way to demonstrate it is:

const fragment: DocumentFragment = <><p>Example</p></>;
//    ^^^^^^^^
// Error: Type 'ReactElement' is not assignable to type 'DocumentFragment'.
// Property 'getElementById' is missing in type 'HTMLElement' but required in type 'DocumentFragment'.ts(2322)

Longer example showing discrepancy between type checking and runtime types:

// Define an example JSX fragment:
const fragment = <><p>Example</p></>;

// Check the TypeScript type of the JSX fragment:
type FragmentType = typeof fragment;
// fragment is inferred as HTMLElement | SVGElement

// But it is actually not HTMLElement | SVGElement ...
console.assert(
  fragment instanceof HTMLElement || fragment instanceof SVGElement,
  "not HTMLElement | SVGElement"
); // Assertion failed: not HTMLElement | SVGElement

// ... but rather DocumentFragment
console.assert(fragment instanceof DocumentFragment); // Assertion passes

Real world problems this might cause would be due to assuming that a component renders as an Element when it could actually be a DocumentFragment. A contrived example -- does not give any type errors or runtime errors, but is clearly wrong (the click listener is not bound or at least does not fire, because DocumentFragments can't have event listeners):

function MyComponent() {
  return <><b id="foo">hello</b> world</>;
}

const x = <MyComponent />;
x.addEventListener('click', () => {
  console.log("click");
});

document.body.appendChild(x);

(I know you would probably not be doing this in real code, you would probably bind the listener using onClick prop, but this is just demonstrating one of the ways that DocumentFragment and Element are different interfaces). Another example might be trying to use getElementsBy* methods which don't exist on DocumentFragment (although I believe it does have getElementById and querySelector).

Finally, another example showing the type error along with a work-around:

// Example of type error
function expectsFragment(fragment: DocumentFragment): void {
  // (... Do something with DocumentFragment)
}
expectsFragment(<><p>Example</p></>); // TypeScript error:
//              ^^^^^^^^^^^^^^^^^^^
// Error: Argument of type 'ReactElement' is not assignable to parameter of type 'DocumentFragment'.
// Property 'getElementById' is missing in type 'HTMLElement' but required in type 'DocumentFragment'.ts(2345)

// Work-around:
function expectsNode(node: Node): void {
  if (node instanceof DocumentFragment) {
    // (... Do something with DocumentFragment)
  }
}
expectsNode(<><p>Example</p></>); // no type error

I hope some of this is helpful, not strictly actual/expected tests, please let me know if you want this in a different format.

[x11x]

Another contrived example if it is any help (just showing difference between components with DocumentFragment vs Element at the root, which you probably already know):

function MyComponentFragment() {
  return <><b>hello</b> world</>;
}

function MyComponentElement() {
  return <div><b>hello</b> world</div>;
}

function test(component: ReactElement): void {
  document.body.append(component);
  console.assert(
    component.parentNode === document.body,
    "component root is not actually in the DOM tree"
  );
}

test(<MyComponentElement />); // No error
test(<MyComponentFragment />); // Assertion failed: component root is not actually in the DOM tree

[x11x]

I'm realising const x: DocumentFragment = <><p>Example</p></>; is maybe never going to work for the same reason that const x: HTMLParagraphElement = <p>Example</p>; and const x: HTMLElement = <p>Example</p>; don't? I think that's what the linked TypeScript issue is about? I initially thought it might be possible to infer the types of fragments different from elements, but unless I'm mistaken every JSX expression is of type JSX.Element which needs to cover every possible type it can be (unless this changes in a future typescript version).

So my first snippet above is probably not useful as a test case.

The issue is more about whether JSX.Element and/or ReactElement should be defined as HTMLElement | SVGElement | DocumentFragment because at runtime this is what it is (as my second snippet demonstrates). And then how to change Component -- it would need to be changed to class Component<P = {}, T extends Element | DocumentFragment = JSX.Element> or maybe better class Component<P = {}, T extends JSX.Element = JSX.Element>. (and then also changing existing code that inherits from Component because T is now a broader type).