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

Add type definitions for the React jsx runtime #64661

Conversation

remcohaszing
Copy link
Contributor

Although one typically shouldn’t use this directly, there are some legitimate use cases for it.

React 15 provides Fragment only through the automatic runtime, not via the index file. The type for Fragment has been backported into the jsx runtime from a newer version.

Select one of these and delete the others:

If changing an existing definition:

Although one typically shouldn’t use this directly, there are some legitimate use cases for it.

React 15 provides `Fragment` only through the automatic runtime, not via the index file. The type for `Fragment` has
been backported into the jsx runtime from a newer version.
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 10, 2023

@remcohaszing Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • 🕐 Only a DT maintainer can approve changes without tests

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 8 days.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 64661,
  "author": "remcohaszing",
  "headCommitOid": "8df849702b776f5a4ff9ff0b1ca1f9913fcaa092",
  "mergeBaseOid": "a885197773d729c49c09e9424a41da0c8e69f544",
  "lastPushDate": "2023-03-10T17:04:31.000Z",
  "lastActivityDate": "2023-04-06T08:49:35.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "react",
      "kind": "edit",
      "files": [
        {
          "path": "types/react/jsx-dev-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/jsx-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v15/jsx-dev-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v15/jsx-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v16/jsx-dev-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v16/jsx-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v17/jsx-dev-runtime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react/v17/jsx-runtime.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "johnnyreilly",
        "bbenezech",
        "pzavolinsky",
        "ericanderson",
        "DovydasNavickas",
        "theruther4d",
        "guilhermehubner",
        "ferdaber",
        "jrakotoharisoa",
        "pascaloliv",
        "hotell",
        "franklixuefei",
        "Jessidhia",
        "saranshkataria",
        "lukyth",
        "eps1lon",
        "zieka",
        "dancerphil",
        "dimitropoulos",
        "disjukr",
        "vhfmag",
        "hellatan",
        "priyanshurav",
        "Semigradsky"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "eps1lon",
      "date": "2023-03-27T18:42:12.000Z"
    },
    {
      "type": "approved",
      "reviewer": "nickrttn",
      "date": "2023-03-23T13:37:57.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1463932644,
  "ciResult": "pass"
}

@typescript-bot typescript-bot added Critical package Untested Change This PR does not touch tests labels Mar 10, 2023
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Mar 10, 2023
@typescript-bot
Copy link
Contributor

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Mar 10, 2023
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Mar 10, 2023
@typescript-bot
Copy link
Contributor

@remcohaszing The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Mar 10, 2023
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Mar 10, 2023
@DangerBotOSS
Copy link

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

react (unpkg)

was missing the following properties:

  1. unstable_act

Generated by 🚫 dangerJS against 8df8497

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board Mar 10, 2023
@typescript-bot typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label Mar 21, 2023
@typescript-bot
Copy link
Contributor

Re-ping @johnnyreilly, @bbenezech, @pzavolinsky, @ericanderson, @DovydasNavickas, @theruther4d, @guilhermehubner, @ferdaber, @jrakotoharisoa, @pascaloliv, @Hotell, @franklixuefei, @Jessidhia, @saranshkataria, @lukyth, @eps1lon, @zieka, @dancerphil, @dimitropoulos, @disjukr, @vhfmag, @hellatan, @priyanshurav, @Semigradsky:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

* You should not use this function directly. Use JSX and a transpiler instead.
*/
export function jsxDEV(
type: ElementType,
Copy link

@nickrttn nickrttn Mar 23, 2023

Choose a reason for hiding this comment

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

ElementType takes a generic P for the type of props. Would it make sense to pass that here as well with the same signature for the generic (P = any)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically I could go even further and implement the same overrides as for createElement(). didn’t want to overcomplicate these types though, as legitimate use cases are rare.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually do want to use these functions' types to typecheck JSX in TS itself, so there would be value in making them more accurate, this way the types would be better when such checking is enabled. Ideally, at least as good as JSX tags are checked today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unrelated to the type checking of JSX syntax. JSX expressions for the automatic runtime are checked using the global JSX namespace. See the TypeScript handbook on JSX. These functions do get used at runtime, so I understand the confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@weswigham Can you share a bit more information how that will look? Specifically with regard to microsoft/TypeScript#14729 (comment)?

In the past, performance concerns were raised. React's JSX factories were never really tested for use in JSX type-checking. To avoid a chicken-egg problem (JSX factories being poorly typed vs TS not consulting JSX factories because poor types), I'd be nice if you could share TS canaries once ready so that I can check how to best type the JSX factories out.

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Mar 23, 2023
Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Do you have a concrete example where these are used today and need types? Ideally as tests added to this repo.

/**
* Create a React element.
*
* You should not use this function directly. Use JSX and a transpiler instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need types for them now? It feels a bit scary to publish them without any actual usage or tests. I'd like to avoid people using them since that's not their intended purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. In a project I work on, we inject our own automatic runtime. This runtime implements the development runtime and uses the positional information it provides, then forwards the input into React’s production runtime. I understand this is a niche use case, but it is a use case nonetheless.
  2. Recently both SolidJS and Vue added support for the JSX automatic runtime. Both implemented it wrong initially. Since React is the de-facto standard JSX implementation, I think this could help other frameworks use it as a point of reference for implementing their own.
  3. This is part of the public interface of React. I think they want you to write JSX and compile. However, when inspecting compiled code, these functions show up.

Of course I’ll gladly add tests, but I prefer to do so after convincing you this should be included at all. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, when inspecting compiled code, these functions show up.

I don't understand how that's relevant to the types. It sounds like you're debugging the transformer plugin and want to use the types to verify?

The larger point is that type-checking JSX is different than normal type-checking in TS. You can't just transform JSX, type-check and expect the same result. I'd like to avoid giving that impression. Otherwise we end up with types like that for createElement which are way more complicated than they need to be.

If TypeScript uses the runtime types for type-checking, we need to rethink their types anyway. I'd rather test this either with their intended design that's going to ship or, if they don't want to collaborate on that, work on it once shipped.

But building them out before just adds more technical debt.

Both implemented it wrong initially.

What mistakes did they make?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case I need it for is the first in the list. I need to wrap React’s automatic runtime, which requires these type definitions.

However, when inspecting compiled code, these functions show up.

I don't understand how that's relevant to the types.

For example, create a new directory, and install a React library:

mkdir project
cd project
npm install @mui/material

Now open ./node_modules/@mui/material/Accordion/Accordion.js. It has these jsx and jsxs variables. TypeScript doesn’t know them, so it shows them as any. I think having these types helps users understand a tiny bit better what these functions are, and what code they’re actually running.

It sounds like you're debugging the transformer plugin and want to use the types to verify?

Not currently, but I have contributed to the JSX transformer in MDX. I see how that could be a potential use case.

The larger point is that type-checking JSX is different than normal type-checking in TS. You can't just transform JSX, type-check and expect the same result. I'd like to avoid giving that impression. Otherwise we end up with types like that for createElement which are way more complicated than they need to be.

I am fully aware how type checking JSX works for both the classic and automatic runtime. I share your sentiment to avoid overcomplication, which is why I kept the types minimal.

If TypeScript uses the runtime types for type-checking, we need to rethink their types anyway. I'd rather test this either with their intended design that's going to ship or, if they don't want to collaborate on that, work on it once shipped.

I’m not aware any work is being done on that.

Both implemented it wrong initially.

What mistakes did they make?

Both basically exported the classic runtime pragma as jsx, jsxs, and jsxDEV. It’s fixed now. I feel this is out of scope for the discussion. My point is these types might help framework authors by providing a reference of how to implement the automatic runtime.

Copy link
Collaborator

@eps1lon eps1lon Apr 6, 2023

Choose a reason for hiding this comment

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

I’m not aware any work is being done on that.

It's scheduled for 5.1 (microsoft/TypeScript#53031) which is why I'm hesitant to work on this now in parallel until we have clarification from the TypeScript team. Which is usually after things have shipped which makes me even more hesitant to continue on this at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't aware of that. Then it makes perfect sense to delay this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll close this PR in favor of whatever implementation will be added once TypeScript 5.1 is released. :)

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Other Approved This PR was reviewed and signed-off by a community member. Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels Mar 27, 2023
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Needs Author Action in New Pull Request Status Board Mar 27, 2023
@typescript-bot
Copy link
Contributor

@remcohaszing One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Revision needed This PR needs code changes before it can be merged. Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants