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) :

jsx-dom/types/index.d.ts

Line 61 in a5682a5

export type ReactElement = HTMLElement | SVGElement

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 microsoft/TypeScript#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).

jsx-dom/src/jsx-dom.ts

Lines 220 to 221 in a5682a5

	} else if (isElement(child)) {
	appendChildToNode(child, node)

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).

jsx-dom/src/util.ts

Lines 13 to 15 in a5682a5

	export function isElement(val: any): val is Element {
	return val && typeof val.nodeType === "number"
	}

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).

[AnYiEE]

I encountered a similar issue related to DocumentFragment(For example, I encountered an error when using the element.remove()), the issue occurred after upgrading from v8.1.4 to v8.1.5. For me, reverting back to v8.1.4 while keeping other changes made everything work fine.

[alex-kinokon]

In TypeScript, JSX can only be a blanket JSX.Element type and there’s no way to tell if it’s a DocumentFragment or an Element (but it will work if you make it a function call instead of JSX, go figure).

@AnYiEE @x11x @yoriiis:
I made ReactElement include DocumentFragment in 8.1.5. This is technically correct, but I reckon that it’s probably annoying since a lot of web APIs only accept Element. Do you think I should revert this change?

[AnYiEE]

In TypeScript, JSX can only be a blanket JSX.Element type and there’s no way to tell if it’s a DocumentFragment or an Element (but it will work if you make it a function call instead of JSX, go figure).

@AnYiEE @x11x @yoriiis: I made ReactElement include DocumentFragment in 8.1.5. This is technically correct, but I reckon that it’s probably annoying since a lot of web APIs only accept Element. Do you think I should revert this change?

I feel that jsx-dom is mainly oriented towards native JavaScript and is mostly used within native JavaScript (jQuery is also considered native JavaScript), rather than trying to emulate React or other comprehensive front-end frameworks and libs. Therefore, it should be as directly compatible with most DOM APIs as possible.

[x11x]

a lot of web APIs only accept Element

At least for mutation, I think most (nearly all?) of the DOM APIs, accept a Node (which might be Element, Text or DocumentFragment).
insertAdjacentElement seems to be an exception (but I guess it makes sense because "Element" is in the name). Its strange that they did not specify a generic "insertAdjacent" method that takes a Node.

In reference to #109, I will point out that the change does protect you against the runtime error caused by:

document.body.insertAdjacentElement('afterbegin', <><h1>Ok</h1></>);  // will raise an error at runtime, since the JSX expression evaluates to a DocumentFragment

If the change was reverted, this would not give any error during type-checking, but would be a runtime error.

Maybe, depending on the project, it would be possible to make sure that nobody uses <></> syntax and therefore this problem would never come up -- but then one day somebody tries to use <></> and finds that things break (that is why I think it is helpful if typescript catches this).

Also, fragments and <></> can be useful, so I believe it is helpful if jsx-dom fully supports them.

Do you think I should revert this change?

I would lean towards correctness (and vote that the change is not reverted), but I totally understand if people find this too annoying or breaks things in a way that is too difficult to fix and want to revert this.

My feeling is that the main reason we use TypeScript is to protect against runtime errors and it is most helpful if it shows us any/all possible runtime errors during type-checking (even if they are initially inconvenient). However, I realize people might have other opinions, and there are maybe other reasons people use TypeScript where correctness is less important for them (for documentation generation, because they are forced to, etc.).

@yoriiis: I know it maybe does not seem ideal but you could use a helper like:

function ensureElement(node: Node): Element {
    if (node instanceof Element) {
        return node;
    }
    throw new TypeError("node must be an Element");
}

Then use it like this:

document.body.insertAdjacentElement('afterbegin', ensureElement(<h1>Ok</h1>));

Also, using APIs that take a Node will obviously work with no extra helpers needed:

// (both of these should work at runtime and have no type-checking errors)
document.body.insertBefore(<h1>Ok</h1>, document.body.firstChild);
document.body.insertBefore(<><h1>Ok</h1></>, document.body.firstChild);

Or if you need insertAdjacentElement semantics but want to support any Node you could write something like:

// (note: untested, but based on algorithm at https://dom.spec.whatwg.org/#insert-adjacent)
function insertAdjacent(position: 'beforebegin' | 'afterbegin' | 'beforeend' | 'afterend', element: Element, newNode: Node): Node | null {
    switch (position) {
        case 'beforebegin':
            const parent = element.parentNode;
            if (parent == null) {
                return null;
            }
            return parent.insertBefore(newNode, element);
        case 'afterbegin':
            return element.insertBefore(newNode, element.firstChild);
        case 'beforeend':
            return element.appendChild(newNode);
        case 'afterend':
            const parent = element.parentNode;
            if (parent == null) {
                return null;
            }
            return parent.insertBefore(newNode, element.nextSibling);
    }
    throw new SyntaxError(`insertAdjacent: invalid position '${position}'`);
}

Then use it like:

insertAdjacent('afterbegin', document.body, <h1>Ok</h1>);
insertAdjacent('afterbegin', document.body, <><h1>Ok</h1></>);

[x11x]

Note also @AnYiEE this issue is all about vanilla DOM API compatibility and has nothing to do with React compatibility (or any other framework). The use of the name "ReactElement" in jsx-dom is just to match the way the JSX runtime was originally implemented in React, it has nothing to do with React or any other framework.

[AnYiEE]

Note also @AnYiEE this issue is all about vanilla DOM API compatibility and has nothing to do with React compatibility (or any other framework). The use of the name "ReactElement" in jsx-dom is just to match the way the JSX runtime was originally implemented in React, it has nothing to do with React or any other framework.

I don't understand why you mentioned me. I believe what I expressed is correct, and at least it conveys the same meaning as what you said. I’m not a native English speaker, but I don’t think I used any strange expressions.

I can understand the solution you provided, but for a large project, making this change would be quite troublesome. There would be many, many places that need to be modified, so even though it is theoretically correct, its practical application may not be as ideal. Also, this solution has a significant impact on convenience.

[x11x]

Sorry, did not mean to offend! Yes, I think we are in agreement.
I agree with your other point too.
My last comment might have been unnecessary, sorry to bother you with the mention.

[yoriiis]

With version 8.1.4 I said I had no errors with the following code, but that's not totally correct.

document.body.insertAdjacentElement('afterbegin', <div>Hello</div>)

In fact, with 8.1.4 and webpack with ts-loader it does not generate an error. But, TSC via VSCode (Problems tab) does display the error (non-blocking).

Argument of type 'React.JSX.Element' is not assignable to parameter of type 'Element'.
  Type 'ReactElement<any, any>' is missing the following properties from type 'Element': attributes, classList, className, clientHeight, and 168 more.

With 8.1.5, webpack with ts-loader generates this one (blocking).

 TS2345: Argument of type 'ReactElement' is not assignable to parameter of type 'Element'.
  Type 'DocumentFragment' is missing the following properties from type 'Element': attributes, classList, className, clientHeight, and 109 more.

---

I feel that jsx-dom is mainly oriented towards native JavaScript and is mostly used within native JavaScript (jQuery is also considered native JavaScript), rather than trying to emulate React or other comprehensive front-end frameworks and libs. Therefore, it should be as directly compatible with most DOM APIs as possible.

I'm agree with @x11x

I know it maybe does not seem ideal but you could use a helper

I don't think this is the right approach.

On https://stackoverflow.com/a/61576091 it is explained that insertAdjacentElement does not accept DocumentFragment and I think it is normal that we cannot use it. But how can we fix this on jsx-dom to work with <div></div> which is not a DocumentFragment ? I have a feeling it is linked to microsoft/TypeScript#21699.

(I apologize in advance because I do not have all the context, I arrive on the subject, I read the issue but the problem is not simple).

[yoriiis]

Hello @alex-kinokon, sorry to interrupt, what do you think about this please? Thank you

/cc @x11x @AnYiEE