-
-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor: Move most interfaces out of the JSX namespace #4878
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
Conversation
94c5bc9 to
832c60d
Compare
| interface ContainerNode { | ||
| export interface ContainerNode { |
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.
Fairly odd: we reference this in /compat/index.d.ts, and that's always been fine, but now it's throwing an error, stating ContainerNode cannot be found. This seems pretty legit to me, seeing as how we weren't exporting it.
| @@ -1,4 +1,4 @@ | |||
| import { createElement, Component, createContext } from '../../'; | |||
| import { createElement, Component, createContext, HTMLAttributes, MouseEventHandler } from '../../'; | |||
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.
MouseEventHandler, like ContainerNode above, seems to be a weird instance of global type access that shouldn't exist. I don't know why that worked, or what has since broke it, but I'd say this is correct?
832c60d to
15bf573
Compare
src/jsx.d.ts
Outdated
| SVGAttributes, | ||
| HTMLAttributes, | ||
| AnnotationMathMLAttributes, | ||
| AnnotationXmlMathMLAttributes, |
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.
This mega import statement is kinda gross but I'm not sure an import * as domTypes is all that much better?
15bf573 to
4514c7c
Compare
205c6b0 to
7f478c4
Compare
Closes #2706
This PR cuts down the
JSXnamespace to only include the actual JSX interfaces, i.e.,Element,IntrinsicElements,IntrinsicAttributes, etc. We've been using it to hold most of our DOM-ish typings forever which is a) surprising for users looking for these types, if they can find them at all and b) probably not ideal given the magic handling ofJSXby TS. Best to keep it pretty minimal and only load it up with the interfaces expected of it.This will let users import types directly from Preact, like so:
Moving these out to a separate declaration file seemed easiest, as we need to re-export all of these types except for the
JSXInternalnamespace and I found that to be quite fiddly; I frequently brokeIntrinsicElementswhen working on that and so just moved the unrelated types out. Seems less error-prone like this, and does somewhat nicely isolate the JSX-specific types. Might be easier to maintain.Big ol' breaking change of course, but for the better I think. Should be pretty quick for consumers to fix up.