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

Change the jsx callback type type to any #4

Closed
wants to merge 1 commit into from
Closed

Conversation

remcohaszing
Copy link
Member

Initial checklist

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

Description of changes

This change makes the automatic runtime of some frameworks assignable to the runtime hast-util-to-jsx-runtime expects.

For example, see what happens if the type of type is toggled in this playground

import {Element} from 'hast'
import {MdxJsxFlowElementHast, MdxJsxTextElementHast} from 'mdast-util-mdx-jsx'
import * as runtime from 'preact/jsx-runtime'
import {JSX} from 'preact'

export type Style = Record<string, string>;
export type Child = JSX.Element | string | null | undefined;
export type Props = {
    children?: Array<Child> | Child;
    [prop: string]: string | number | boolean | Element | MdxJsxTextElementHast | MdxJsxFlowElementHast | JSX.Element | Style | Child[] | null | undefined;
};

export type Fragment = unknown;
export type Jsx = (
    // type: unknown,
    type: any,
    props: Props,
    key?: string | undefined
) => JSX.Element;

export type RuntimeProduction = {
    Fragment: Fragment;
    jsx: Jsx;
    jsxs: Jsx;
};

const foo: RuntimeProduction = runtime

type-coverage fails for this PR. I see 2 possible solutions:

  1. Move the Jsx and JsxDev definitions into a .d.ts file, so we can add a type-coverage:ignore-next-line comment.
  2. Disable type-coverage. Personally I don’t find it very useful anyway.

This change makes the automatic runtime of some frameworks assignable to
the runtime `hast-util-to-jsx-runtime` expects.
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Feb 13, 2024
@ChristianMurphy
Copy link
Member

@remcohaszing this feels a bit like a JSX typing bug in preact? Would this be something to address upstream?

To the question on tooling, I'm for keeping type-coverage, in my opinion it provides useful feedback when reviewing type related PRs.

@remcohaszing
Copy link
Member Author

The Preact types are completely correct. This is an assignability issue caused by the unknown here.

Anything is assignable to any or unknown. any is assignable to anything. unknown is only assignable to any or unknown.

A function that accepts unknown, is assignable to a function that accepts more correctly typed argument, but not the other way around. playground link

type UnknownFn = (arg: unknown) => unknown
type StringFn = (arg: string) => unknown

declare let unknownFn: UnknownFn
declare let stringFn: StringFn

// This is a type error
unknownFn = stringFn
stringFn = unknownFn

The situation for hast-util-to-jsx-runtime is the same. It accepts an object with functions whose argument is unknown, but the implementation has a more strictly typed signature.

@remcohaszing
Copy link
Member Author

To the question on tooling, I'm for keeping type-coverage, in my opinion it provides useful feedback when reviewing type related PRs.

IMO typescript-eslint is better suited for this. I think supports pretty much all features we use from type-coverage.

But I don’t want to focus on that as part of this PR.

@wooorm
Copy link
Member

wooorm commented Feb 13, 2024

Is this issue about compatibility with Preact? Which frameworks don’t work? That’s quite possible but perhaps they need to be tested, and perhaps we can solve it some other way?

Here’s the Preact types: https://github.com/preactjs/preact/blob/main/jsx-runtime/src/index.d.ts

@darthmaim
Copy link

@types/react@18.2.59 added types for jsx and jsxs. I think this PR would resolve the following error that now occurs after updating.

Type error: Type '(type: ElementType<any, keyof IntrinsicElements>, props: unknown, key?: Key | undefined) => ReactElement<any, string | JSXElementConstructor<any>>' is not assignable to type 'Jsx'.
  Types of parameters 'type' and 'type' are incompatible.
    Type 'unknown' is not assignable to type 'ElementType<any, keyof IntrinsicElements>'.

  31 |
  32 |   const tree = starryNight.highlight(code, scope);
> 33 |   const reactNode = toJsxRuntime(tree, { Fragment, jsx, jsxs });
     |                                                    ^
  34 |
  35 |   return reactNode;
  36 | };

@wooorm
Copy link
Member

wooorm commented Feb 27, 2024

See also #6.

@remcohaszing
Copy link
Member Author

Closed in favor of #6.

@remcohaszing remcohaszing deleted the jsx-type-any branch March 18, 2024 13:05

This comment has been minimized.

@remcohaszing remcohaszing added the 🤷 no/invalid This cannot be acted upon label Mar 18, 2024
@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤷 no/invalid This cannot be acted upon 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

None yet

4 participants