-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix confusing type errors by reversing order of overloaded h() definitions #1214
Fix confusing type errors by reversing order of overloaded h() definitions #1214
Conversation
For overloaded functions, TypeScript reports errors based on the last of the overloaded definitions. For `h`'s overloaded definitions, the string version is currently last. When type checking of an `h` call fails (for example, due to a missing prop, or due to an incorrect prop type), TypeScript simply says "Argument of type 'typeof OurComponent' is not assignable to parameter of type 'string'." This patch switches the order of the `h` definitions, so that the h<P> version comes last. This makes TypeScript report a more specific error that will include missing props, etc.
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.
Seems good to me.
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'd love to verify this first. Usually the last definition in TS represents the actual implementation in an overloaded function. A third combined definition is missing here. Switching them around would only disable one of them, but I'll have to check to be sure.
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.
Just tried out the changes in the PR but they fail for this test case:
import { h } from "preact";
class Foo extends Component {
render() {
return <div>Hello</div>
}
}
function Bar() {
return <div>World</div>
}
h("div", {}, h(Foo, null), h(Bar, null));
with the following error message:
[ts] Argument of type 'typeof Foo' is not assignable to parameter of type 'string'.
I can't reproduce that failure, but I also don't use JSX and have since given up on TypeScript. |
This PR fixes it for me |
@marvinhagemeister I misunderstood your comment before. I think you mean that even with this patch, the uninformative error message is sometimes shown? Unfortunately I don't know how to install my own preact fork as a dev dependency (it's not requireable when I try), so I can't test whether this PR at least improves the frequency of the "good" error message showing up. This issue is far and away my biggest annoyance when using preact with typescript, so it would be really good to have it fixed. Unfortunately, changes beyond the simple reordering in this PR are well beyond my meager understanding of TS at this point. Have you given any thought to adding that third combined definition that you mentioned? |
#1246 adds a test for this and another type fix for non-jsx preact |
Ahh, that sounds great. There's also a TypeScript issue to fix the TS error message itself (microsoft/TypeScript#27249), but who knows when that will land. |
Just looked at this issue again and I can confirm that it indeed does fix the issue. Not sure why I was getting a failure when previously reviewing the changes. On top of that I stand corrected with the number of overloads declared inside a declaration file. I was under the impression that they had to be declared the same way in a @garybernhardt I'm really thankful that you brought this issue up 👍 Issues like these are not easy to spot :) Congratulations for your first contribution to preact 👍 🎉 |
Thanks for merging! |
For overloaded functions, TypeScript reports errors based on the last of the overloaded definitions. For
h
's overloaded definitions, the string version is currently last. When type checking of anh
call fails (for example, due to a missing prop, or due to an incorrect prop type), TypeScript simply says "Argument of type 'typeof OurComponent' is not assignable to parameter of type 'string'." This patch switches the order of theh
definitions, so that theh<P>
version comes last. This makes TypeScript report a more specific error that will include missing props, etc.Before this change:
After this change:
See my working example repo. Switch the order of the
h
definitions insrc/preact-8.2.9.min.d.ts
in that repo and you'll see that the error goes from the first (confusing) error above to the second (very clear) error.