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

fix: do not use JSX.ElementChildrenAttribute under jsx: react-jsx or jsx: react-jsxdev #60880

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

uhyo
Copy link
Contributor

@uhyo uhyo commented Dec 30, 2024

Happy Holidays! Fixes #60572

🖼️ Background

Under jsx: react-jsx or jsx: react-jsxdev, the name of the prop for JSX children is always children. The jsx transformer properly implements this:

return result && factory.createPropertyAssignment("children", result);

However, the type checker didin't take this into consideration. Instead, it used JSX.ElementChildrenAttribute to determine the name of prop. If type definition lacks this interface, undesirable compiler errors are emitted (like the one mentioned in the linked issue).

It is of note that, I believe, no practical type definition for JSX libraries ever provided such definition that would lead to errors like those.

That said, I think the current behavior is still a bit harmful to those who want to learn how to write type definitions for JSX libraries, so proposing a fix.

🦾 Solution

This PR fixes the getJsxElementChildrenPropertyName function so that children is hard-coded under these jsx modes.

🌵 Current Behavior

code:

//@filename: /jsx.d.ts
declare namespace JSX {
  interface IntrinsicElements {
    h1: { children: string }
  }

  type Element = string;
}

//@filename: /test.tsx
const Title = (props: { children: string }) => <h1>{props.children}</h1>;

const element = <Title>Hello, world!</Title>;

Errors:

    const Title = (props: { children: string }) => <h1>{props.children}</h1>;
                                                    ~~
!!! error TS2741: Property 'children' is missing in type '{}' but required in type '{ children: string; }'.
!!! related TS2728 /jsx.d.ts:3:11: 'children' is declared here.

    const element = <Title>Hello, world!</Title>;
                     ~~~~~
!!! error TS2741: Property 'children' is missing in type '{}' but required in type '{ children: string; }'.
!!! related TS2728 /test.tsx:1:25: 'children' is declared here.

✨ New Behavior

(No error)

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 30, 2024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be good to add a test case that defines ElementChildrenAttribute as something different than children to verify that it's ignored in favor of "hard-coded" children

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Added a test case.

@jakebailey
Copy link
Member

@typescript-bot test it

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 18, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/60880/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 34 34 ~ ~ ~ p=1.000 n=6
Symbols 62,390 62,390 ~ ~ ~ p=1.000 n=6
Types 50,395 50,395 ~ ~ ~ p=1.000 n=6
Memory used 196,172k (± 0.84%) 195,623k (± 0.99%) ~ 193,002k 196,909k p=0.810 n=6
Parse Time 1.62s (± 0.64%) 1.61s (± 0.61%) ~ 1.60s 1.62s p=0.437 n=6
Bind Time 0.87s (± 1.34%) 0.88s (± 1.56%) ~ 0.86s 0.90s p=0.563 n=6
Check Time 11.78s (± 0.57%) 11.79s (± 0.72%) ~ 11.70s 11.93s p=1.000 n=6
Emit Time 3.32s (± 3.48%) 3.33s (± 3.54%) ~ 3.25s 3.56s p=0.872 n=6
Total Time 17.59s (± 0.68%) 17.61s (± 0.98%) ~ 17.43s 17.91s p=1.000 n=6
angular-1 - node (v18.15.0, x64)
Errors 37 37 ~ ~ ~ p=1.000 n=6
Symbols 947,969 947,969 ~ ~ ~ p=1.000 n=6
Types 410,955 410,955 ~ ~ ~ p=1.000 n=6
Memory used 1,225,865k (± 0.01%) 1,225,847k (± 0.00%) ~ 1,225,737k 1,225,902k p=0.575 n=6
Parse Time 8.05s (± 0.54%) 8.08s (± 1.00%) ~ 7.94s 8.14s p=0.261 n=6
Bind Time 2.29s (± 0.89%) 2.28s (± 0.82%) ~ 2.26s 2.31s p=0.286 n=6
Check Time 38.23s (± 0.31%) 38.16s (± 0.42%) ~ 37.94s 38.37s p=0.572 n=6
Emit Time 18.37s (± 0.33%) 18.28s (± 0.57%) ~ 18.21s 18.46s p=0.170 n=6
Total Time 66.93s (± 0.18%) 66.80s (± 0.11%) -0.13s (- 0.20%) 66.66s 66.88s p=0.045 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,444,602 2,444,602 ~ ~ ~ p=1.000 n=6
Types 896,634 896,634 ~ ~ ~ p=1.000 n=6
Memory used 2,305,803k (± 0.00%) 2,305,854k (± 0.00%) ~ 2,305,700k 2,306,005k p=0.230 n=6
Parse Time 10.78s (± 0.52%) 10.80s (± 0.50%) ~ 10.73s 10.89s p=0.688 n=6
Bind Time 2.53s (± 0.61%) 2.54s (± 0.63%) ~ 2.51s 2.55s p=0.365 n=6
Check Time 88.18s (± 0.86%) 88.51s (± 1.93%) ~ 87.27s 91.26s p=0.575 n=6
Emit Time 0.35s (± 2.98%) 0.35s (± 2.14%) ~ 0.34s 0.36s p=0.437 n=6
Total Time 101.84s (± 0.71%) 102.20s (± 1.66%) ~ 100.87s 104.89s p=0.689 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,459 1,228,459 ~ ~ ~ p=1.000 n=6
Types 266,873 266,873 ~ ~ ~ p=1.000 n=6
Memory used 2,971,149k (±10.01%) 2,996,101k (± 7.83%) ~ 2,516,543k 3,092,967k p=0.689 n=6
Parse Time 8.34s (± 1.32%) 8.34s (± 0.97%) ~ 8.21s 8.40s p=1.000 n=6
Bind Time 2.65s (± 2.50%) 2.65s (± 1.11%) ~ 2.62s 2.70s p=0.936 n=6
Check Time 53.21s (± 0.43%) 53.19s (± 0.83%) ~ 52.35s 53.67s p=1.000 n=6
Emit Time 4.28s (± 3.33%) 4.27s (± 1.67%) ~ 4.17s 4.34s p=1.000 n=6
Total Time 68.49s (± 0.33%) 68.45s (± 0.80%) ~ 67.41s 69.03s p=0.810 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,228,459 1,228,459 ~ ~ ~ p=1.000 n=6
Types 266,873 266,873 ~ ~ ~ p=1.000 n=6
Memory used 3,036,422k (± 9.76%) 2,673,339k (±14.00%) ~ 2,430,659k 3,157,378k p=0.173 n=6
Parse Time 7.06s (± 0.80%) 6.87s (± 1.42%) -0.19s (- 2.67%) 6.75s 6.98s p=0.005 n=6
Bind Time 2.13s (± 1.47%) 2.15s (± 0.96%) ~ 2.13s 2.18s p=0.518 n=6
Check Time 42.95s (± 0.52%) 42.86s (± 0.47%) ~ 42.67s 43.13s p=0.471 n=6
Emit Time 3.50s (± 2.05%) 3.43s (± 2.32%) ~ 3.36s 3.58s p=0.173 n=6
Total Time 55.65s (± 0.51%) 55.31s (± 0.58%) ~ 55.05s 55.81s p=0.128 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 262,628 262,628 ~ ~ ~ p=1.000 n=6
Types 106,697 106,697 ~ ~ ~ p=1.000 n=6
Memory used 440,819k (± 0.03%) 440,779k (± 0.02%) ~ 440,659k 440,971k p=0.575 n=6
Parse Time 3.56s (± 0.81%) 3.54s (± 0.61%) ~ 3.52s 3.57s p=0.625 n=6
Bind Time 1.32s (± 1.07%) 1.32s (± 0.93%) ~ 1.30s 1.33s p=0.564 n=6
Check Time 18.98s (± 0.26%) 19.02s (± 0.54%) ~ 18.90s 19.17s p=0.470 n=6
Emit Time 1.52s (± 0.99%) 1.52s (± 1.27%) ~ 1.49s 1.54s p=1.000 n=6
Total Time 25.38s (± 0.25%) 25.41s (± 0.47%) ~ 25.28s 25.57s p=0.936 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 70 70 ~ ~ ~ p=1.000 n=6
Symbols 226,095 226,095 ~ ~ ~ p=1.000 n=6
Types 94,488 94,488 ~ ~ ~ p=1.000 n=6
Memory used 371,625k (± 0.01%) 371,624k (± 0.01%) ~ 371,556k 371,679k p=0.810 n=6
Parse Time 2.33s (± 0.35%) 2.32s (± 0.57%) ~ 2.31s 2.34s p=0.507 n=6
Bind Time 1.33s (± 0.61%) 1.35s (± 1.75%) ~ 1.33s 1.38s p=0.270 n=6
Check Time 13.71s (± 0.25%) 13.71s (± 0.28%) ~ 13.66s 13.77s p=1.000 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 17.37s (± 0.18%) 17.39s (± 0.23%) ~ 17.35s 17.46s p=0.629 n=6
vscode - node (v18.15.0, x64)
Errors 3 3 ~ ~ ~ p=1.000 n=6
Symbols 3,251,957 3,251,957 ~ ~ ~ p=1.000 n=6
Types 1,119,191 1,119,191 ~ ~ ~ p=1.000 n=6
Memory used 3,317,206k (± 0.00%) 3,317,155k (± 0.00%) ~ 3,317,074k 3,317,223k p=0.229 n=6
Parse Time 11.58s (± 0.20%) 11.59s (± 0.48%) ~ 11.52s 11.69s p=0.686 n=6
Bind Time 3.80s (± 0.49%) 3.79s (± 0.32%) ~ 3.78s 3.81s p=0.323 n=6
Check Time 73.99s (± 0.30%) 74.01s (± 0.23%) ~ 73.90s 74.32s p=0.936 n=6
Emit Time 23.34s (± 0.17%) 23.32s (± 0.25%) ~ 23.22s 23.38s p=0.746 n=6
Total Time 112.71s (± 0.23%) 112.72s (± 0.11%) ~ 112.61s 112.88s p=0.575 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 291,595 291,595 ~ ~ ~ p=1.000 n=6
Types 118,971 118,971 ~ ~ ~ p=1.000 n=6
Memory used 445,211k (± 0.01%) 445,216k (± 0.01%) ~ 445,152k 445,274k p=0.936 n=6
Parse Time 3.29s (± 0.50%) 3.29s (± 0.49%) ~ 3.27s 3.31s p=0.623 n=6
Bind Time 1.48s (± 0.85%) 1.47s (± 1.26%) ~ 1.45s 1.50s p=0.565 n=6
Check Time 15.67s (± 0.16%) 15.65s (± 0.29%) ~ 15.58s 15.70s p=0.687 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 20.43s (± 0.13%) 20.42s (± 0.29%) ~ 20.33s 20.47s p=1.000 n=6
xstate-main - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 555,050 555,050 ~ ~ ~ p=1.000 n=6
Types 186,115 186,110 -5 (- 0.00%) ~ ~ p=0.001 n=6
Memory used 493,996k (± 0.02%) 494,035k (± 0.01%) ~ 493,985k 494,070k p=0.575 n=6
Parse Time 3.41s (± 0.48%) 3.41s (± 0.60%) ~ 3.39s 3.45s p=0.669 n=6
Bind Time 1.18s (± 1.07%) 1.19s (± 0.68%) ~ 1.18s 1.20s p=0.062 n=6
Check Time 19.72s (± 1.98%) 19.63s (± 0.47%) ~ 19.47s 19.72s p=0.377 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.31s (± 1.61%) 24.23s (± 0.40%) ~ 24.08s 24.35s p=0.521 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/60880/merge:

Everything looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Not started
Development

Successfully merging this pull request may close these issues.

Confusing error when forgot to define JSX.ElementChildrenAttribute type
4 participants