-
-
Notifications
You must be signed in to change notification settings - Fork 885
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 support for React 19 #879
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
index.js:4
- [nitpick] The re-addition of the ExtraProps typedef seems redundant and could be confusing. Consider removing it if not necessary.
* @typedef {import('./lib/index.js').ExtraProps} ExtraProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @remcohaszing
// @ts-expect-error | ||
// React components are allowed to return numbers, | ||
// but not according to the types in hast-util-to-jsx-runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to syntax-tree/hast-util-to-jsx-runtime#6 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat yes. But also apparently hast-util-to-jsx-runtime
doesn’t support JSX.ElementType
, which JSX runtimes may optionally define. That adds another complexity on top of that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to solve such things instead of ts-expect-error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but that issue has been proven hard to resolve upstream. (If it can be resolved upstream at all.) This @ts-expect-error
is internal anyway, and this change unblocks a lot of users.
Let's make this happen! |
Why does |
/** | ||
* @typedef {{ | ||
* [Key in Extract<ElementType, string>]?: ElementType<ComponentProps<Key> & ExtraProps> | ||
* }} Components | ||
* Map tag names to components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this type is not equivalent to the previous Components
.
But this is listed as a patch.
Is it intentional that you remove keyof JSX.IntrinsicElements
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ElementType
includes JSX.IntrinsicElements
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@remcohaszing No, I mean keyof
? Does this allow b: 'i'
. I assume not, because i
or all other element names are not related to props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does! And there’s also a pre-existing test which covers this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I do not see it in the source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 523 in 27d3949
renderToStaticMarkup(<Markdown children="# a" components={{h1: 'h2'}} />), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right no that, I meant in the @types/react
code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like it could be a bug that @types/react
supports this today — that they could remove it somewhere in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure where it happens but when I change that line to h1: 'x-y'
we get a type error — which indicates that I should worry less because it is type checked — even though the types should likely allow custom elements like that.
// @ts-expect-error | ||
// React components are allowed to return numbers, | ||
// but not according to the types in hast-util-to-jsx-runtime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to solve such things instead of ts-expect-error
What a very somber and philosophical question 😆 If you are asking why it appeared on this PR as a reviewer. |
This adds support for React 19 types, by removing the dependency on the types provided by hast-util-to-jsx-runtime, thus removing the dependency on the JSX global namespace.
And, are you happy with it? 😅 I am emboldened in my view that I am not happy with it 😅 |
I’m fine with @ChristianMurphy wanting to try Copilot reviews, but IMO the result is really pointless, at least in this case. |
Of course! Always up for trying things out! |
This comment has been minimized.
This comment has been minimized.
It's not perfect, but getting better.
Okay
It was in this case. |
Why not reuse this solution: quantizor/markdown-to-jsx@918b44b#diff-4b76aeed93d86ab1e483e12333e1bae64ccdc1a1e4fd27bec2b32f41db08e6cfR5 and just change: to in complex-types.ts and complex-types.d.ts? |
The goal of |
Released in |
done in |
Initial checklist
Description of changes
This adds support for React 19 types, by removing the dependency on the types provided by hast-util-to-jsx-runtime, thus removing the dependency on the JSX global namespace.
Unfortunately the current setup doesn’t really allow testing the types against multiple React versions. #838 would enable that.
Closes #877