Fix the JSX runtime types in RunOptions

[remcohaszing]

Initial checklist

• [x] I read the support docs
• [x] I read the contributing guide
• [x] I agree to follow the code of conduct
• [x] I searched issues and couldn’t find anything (or linked relevant results below)
• [x] If applicable, I’ve added docs and tests

Description of changes

@types/react now has types for react/jsx-runtime and react/jsx-dev-runtime. These types are not compatible with the types provided by hast-util-to-jsx-runtime, which are used by MDX.

To resolve the issue, all runtime related options have been changed to unknown. Since the user is supposed to pass in whatever JSX runtime they imported, this should be sufficient. This removes the dependency on hast-util-to-jsx-runtime.

This fixes several type errors. An outdated workaround has been removed from the documentation.

Closes #2463

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 16, 2024 3:22pm

[remcohaszing]

The other PR only changed that they’re no longer exported from internal files. It could be considered breaking here. There’s not really a use for them anymore, but they can be defined as unknown to avoid breaking existing imports.

On the other hand, as a user I wouldn’t find it too bad if TypeScript forces me to make this change:

- /**
-  * @import {Fragment, Jsx} from '@mdx-js/mdx'
-  */
-
- import * as runtime_ from 'react/jxs-runtime'
-
- const runtime = /** @type {{Fragment: Fragment, jsx: Jsx, jsxs: Jsx}} */ (
-   /** @type {unknown} */ (runtime_)
- )
+ import * as runtime from 'react/jxs-runtime'

[wooorm]

I think we’ve had this conversation partially a couple times already, the gist of my stance is: generally, we have types for things. so why are we now removing types? I don’t consider hiding type errors by treating everything as unknown an improvement. These things are at least functions. And probably stricter than that. Not unknown/any?

[remcohaszing]

I think we should aim at the following:

1. :heavy_check_mark: Working runtime code.
2. ❌ Types that make user code at least compile with TypeScript.
3. ❌ Types that provide more helpful type checks.

While (3) is great, I strongly believe we need to aim for (2) first. That’s what this PR solves. I don’t think any users are helped with types that prevent the user from using the code without type casting (to / from unknown even) or @ts-ignore checks.

I doubt it’s useful, because users are typically supposed to pass in a known runtime, not define one. However, it is possible to use the following somewhat stricted types:

/**
 * @typedef {unknown} Fragment
 */

/**
 * @callback Jsx
 * @param {any} type
 * @param {object} props
 * @param {any} [key]
 */

/**
 * @callback JsxDev
 * @param {any} type
 * @param {object} props
 * @param {any} key
 * @param {boolean} isStatic
 * @param {object} [source]
 * @param {object} [self]
 */

• Fragment: was already unknown.
• type: must be any. The real type of type is defined by the JSX runtime. It can’t be unknown, because the type of type must be assignable to what the JSX runtime expects.
• props: must be an object.
• key: same thing as type
• isStatic: of this we actually know it must be a boolean.
• source: we know the shape, it might be a bit dangerous to assume the type provided by the JSX runtime is compatible.
• self: is always an object.

The problem with those types is that type-coverage complains about some of the arguments being any. So CI fails.

[wooorm]

I strongly believe we need to aim for (2) first

Why? I don‘t think we’ve done that before: implement an any instead of actually solving types. Why do you want to implement a simple workaround instead of solving this bug?

I doubt it’s useful, because users are typically supposed to pass in a known runtime, not define one. However, it is possible to use the following somewhat stricted types:

I do think that people should be able to type check jsx/jsxs/Fragment.

Also keep in mind that these types are from another project where this is important. It’s important in several of our projects. We can solve it once, in a good way?

The problem with those types is that type-coverage complains about some of the arguments being any. So CI fails.

That sounds like something that can be solved?

[remcohaszing]

I strongly believe we need to aim for (2) first

Why? I don‘t think we’ve done that before: implement an any instead of actually solving types. Why do you want to implement a simple workaround instead of solving this bug?

TypeScript is supposed to help users. Types that are wrong are significantly worse than types that are loose. What you call a simple workaround, I call an imperfect, but pragmatic solution.

We actually do use any in a couple of places in this repo. It happens in places where we act as the user. This PR removes those occurrences.

• In docs/_asset/editor.jsx we use @ts-expect-error to suppress a type error. This is a somewhat worse version of casting it to any.
• In docs/migrating/v3.mdx we tell our users to use @ts-expect-error, which again is a worse version of casting to any.
• The same happens in packages/mdx/test/evaluate.js
• In packages/react/test/index.jsx we cast as unknown as SomethingElse, which is the same as as any as SomethingElse.

I doubt it’s useful, because users are typically supposed to pass in a known runtime, not define one. However, it is possible to use the following somewhat stricted types:

I do think that people should be able to type check jsx/jsxs/Fragment.

Sure, it’s nice, but the user is bound to the types of their JSX framework and MDX. They just import one thing and pass it to a function. They don’t have a say in how any of it is typed or implemented. I’m not saying stricter types are bad, I’m saying the status quo (incorrect types) is significantly worse than loose types.

Also keep in mind that these types are from another project where this is important. It’s important in several of our projects. We can solve it once, in a good way?

hast-util-to-jsx-runtime is only used in MDX for its (incorrect) types. Yes, the problem needs to be solved there as well, but there’s no reason to block a fix in MDX for that.

The problem with those types is that type-coverage complains about some of the arguments being any. So CI fails.

That sounds like something that can be solved?

I don’t see how, except for disabling type-coverage, at least for the file that contains the any.

[wooorm]

They just import one thing and pass it to a function.

I disagree. Framework developers are also humans: they might want to check whether their runtime works with MDX. There are also tools that are not frameworks, but wrap JSX runtimes: think emotion, restyle. They might want to check whether their runtime works with MDX. And finally there are humans that for other reasons just wrap JSX runtimes, such as for debugging or to learn.

They don’t have a say in how any of it is typed or implemented.

I disagree, I think they do.

I’m saying the status quo (incorrect types) is significantly worse than loose types.

Facebook did not ship types. It was a @ts-expect-error. Now they ship types. It is a @ts-expect-error.

hast-util-to-jsx-runtime is only used in MDX for its (incorrect) types.

This is intentional: if we can get correct types from another place that is fine too. I argue that most of the bugs we uncovered in JSX runtimes (svelte, solid, vue, preact) would’ve been improved if there were some good types/docs for the automatic runtime.

Yes, the problem needs to be solved there as well, but there’s no reason to block a fix in MDX for that.

We often implement solutions in the place where the problem originates, instead of patching intermediate packages?

I don’t see how, except for disabling type-coverage, at least for the file that contains the any.

type-coverage supports ignore comments. type-coverage also supports choosing which files to ignore: you can put anys in a particular file and ignore only that file.

[wooorm]

Externally fixed, thanks!