-
-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: createElement
& h
types
#4578
Conversation
type: keyof JSXInternal.IntrinsicElements, | ||
type: keyof JSXInternal.IntrinsicSVGElements, |
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.
Sorry, drive-by change.
I couldn't find a situation in which this actually made a difference, as createElement
/h
seemingly always falls back to a very loose overload that allows anything, but we extracted out IntrinsicSVGElements
a bit more than a year ago and so we should be using it.
Let's add a ts-test |
Unfortunately our ts-tests seem to be not working correctly. This is an error in the TS playground, but not in our test suite: h('a', { href: 'https://example.com' }); I have no earthly idea why at the moment, hence why I held off on a test. I don't understand what differentiates |
The TS-playground test from the issue replicated for me in our tests, I based my comments off of those tests |
Yeah that one does for me too, but seemingly not other equivalent tests? Which is why I'm confused. I can copy/paste that and move on, just don't understand what exactly is diverging & what we're testing. Something seems off. |
Hmm, that's odd, the type-checking used to work very well and I think with the examples I gave still does. Not sure what changed though, to your point |
OH nvm, it's falling into the |
Closes #4577
Seems like a bit of a shame, essentially the type falls through to just match against a string and offer no real type checking, but it's seemingly what React does.