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

Slow Performance with Type Inference on JSX Element #43717

Closed
scottwillmoore opened this issue Apr 17, 2021 · 11 comments
Closed

Slow Performance with Type Inference on JSX Element #43717

scottwillmoore opened this issue Apr 17, 2021 · 11 comments
Labels
Not a Defect This behavior is one of several equally-correct options
Milestone

Comments

@scottwillmoore
Copy link

Version
TypeScript 4.2.4
TypeScript 4.3.0-dev.20210417

Expected Behaviour
Any changes made to the Box component have fast validation and auto-completion for associated attributes.

Actual Behaviour
TypeScript validation and auto-completion is slow.

Description
I have been investigating the current trend to create a generic React element which will pass through an element property and associated attributes and call React.createElement. An approach similar to this is used by several prominent projects such as Material UI and Github Primer. I wanted to ensure that TypeScript will infer the correct attributes from the given element property. The code I wrote does work, however it does appear to have a significant performance impact.

Related
This issue may be related to these issues: #34801, #43485, #42010, #14729, #38583.

Example
A small, reproducible example can be found at: https://github.com/scottwillmoore/react-box. You can also experiment with it in CodeSandbox. The CodeSandbox version has some differences.

The meat of the TypeScript can be seen below:

import React from "react";

type ElementType = keyof JSX.IntrinsicElements;

type ElementProperties<T extends ElementType> = JSX.IntrinsicElements[T];

type WithElement<T extends ElementType> = {
    element: T;
} & ElementProperties<T>;

type WithChildren = {
    children?: React.ReactNode;
};

type BoxProperties<T extends ElementType> = WithElement<T> & WithChildren;

function Box<T extends ElementType>({ element, children, ...properties }: BoxProperties<T>) {
    return React.createElement(element, properties, children);
}

export default function App() {
    return (
        <Box element="a" href="#">
            Hello, world!
        </Box>
    );
}
@scottwillmoore
Copy link
Author

Quick update, I have changed the implementation to:

type Join<T, U> = T & Omit<U, keyof T>;

type As = React.ElementType;

type AsProps<T extends As> = React.ComponentPropsWithoutRef<T>;

type WithAs<T extends As> = Join<{ as: T }, AsProps<T>>;

type WithChildren = {
    children?: React.ReactNode;
};

type BoxProps<T extends As> = WithAs<T> & WithChildren;

function Box<T extends As>({ as, children, ...properties }: BoxProps<T>) {
    return React.createElement(as, properties, children);
}

export default function App() {
    return (
        <Box as="a" href="#">
            Hello, world!
        </Box>
    );
}

This is based on the Chakra UI implementation and it appears to significantly speed up the validation and auto-completion. It also does not work if you exclude Join, which I feel shouldn't make a difference...

Even when simplified to:

type BoxProps<T extends As> = { as: T } & Omit<React.ComponentPropsWithoutRef<T>, never>;

As soon as the Omit is removed, the inference does not work. Does anyone know why this may occur?

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Apr 20, 2021
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Apr 20, 2021
@amcasey
Copy link
Member

amcasey commented Apr 20, 2021

Check times:
TS 3.7 - 1.0s
TS 3.8 - 1.1s
TS 3.9 - 1.5s

It seems pretty stable both before and after. Note that even 3.7 is pretty slow for this amount of code.

@amcasey
Copy link
Member

amcasey commented Apr 20, 2021

#41821 doesn't help.

@scottwillmoore
Copy link
Author

Oh sorry, I should've mentioned it is less of a performance regression, but more of an area for improvement since this pattern appears quite popular in several popular React component libraries. I will look into #41821, it does look similar. Also, did you have any idea why omission of Omit from my second example appears to cause type inference to not work?

@amcasey
Copy link
Member

amcasey commented Apr 21, 2021

No worries, checking for regressions is just a normal part of perf investigations.

Just to set expectations, ElementType is a huge union and that will almost always lead to perf problems. I'll look in case something is improvable in this particular code, but, as you saw in the bugs you linked, this has already received a lot of attention.

I'm not very familiar with these types, so you'll need to be more specific with your question about emit. I tried using your second implementation with the replacement for BoxProps that you suggested and then removed the href attribute from the <Box> element and completion suggested adding it back. Did you mean something else?

Edit: I forgot to remove the Omit. 🤦‍♂️

@amcasey
Copy link
Member

amcasey commented Apr 22, 2021

Here's a simpler version, but I still don't know why there's an error:

type ComponentProps<T extends "symbol"> =
    T extends string
        ? JSX.IntrinsicElements[T]
        : never;

function F<T extends "symbol">(p1: Pick<ComponentProps<T>, keyof ComponentProps<T>>, p2: ComponentProps<T>) {
    p1 = p2; // Fine
    p2 = p1; // Error
}

@amcasey
Copy link
Member

amcasey commented Apr 22, 2021

For my simplified version, at least, I believe the issue is that the compiler is not taking advantage of the fact that T extends "symbol" implies T extends string, so ComponentProps<T> will never be never (if it were, p2 = p1 would be an error).

So that explains why Omit<Type1, never> is not the same as Type1. I haven't followed all the way through to figure out why it affects IntelliSense in your example, but I believe the answer will be similar - the compiler isn't absolutely sure of the type, so it doesn't offer the members.

@scottwillmoore
Copy link
Author

That is really weird, thank you for looking into this issue. I think I follow your logic, if T extends "symbol", the compiler does not know that T extends string is implied and therefore p2: ComponentProps<T> where T extends "symbol" conditionally becomes never. I still don't quite understand how Omit fixes this, but I had some free time to have a quick look through other TypeScript issues and found this issue #36981 which led me to this pull request #42524, which may, or may not be related.

@audunolsen
Copy link

Dunno why or how, but for me I just picked the intrinsic elements that I am most likely to use and that fixed the sluggish performance

type Elements = Pick<
  JSX.IntrinsicElements,
  'a' | 'div' | 'button' | 'span' | 'article' | 'form' | 'input'
>;

@RyanCavanaugh RyanCavanaugh removed their assignment Feb 8, 2024
@RyanCavanaugh RyanCavanaugh added Not a Defect This behavior is one of several equally-correct options and removed Needs Investigation This issue needs a team member to investigate its status. labels Feb 8, 2024
@RyanCavanaugh
Copy link
Member

Taking a look at this in 5.4, I added 300 lines of this to the project

void <Box element="a" href="#"></Box>;

and check time was 1.44 seconds. Going from 300 lines of it to ~6,000 lines of it, the check time increased to 4.32 seconds, implying a per-element cost of 0.48ms. That seems a little on the high side but not really in the "defect" level of performance considering that Box has to compute the fairly intensive question of "What are all the possible HTML elements and their respective attributes?".

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Not a Defect" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants