-
-
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
Add boolean to ComponentChild in preact.d.ts and remove object from type #1219
Add boolean to ComponentChild in preact.d.ts and remove object from type #1219
Conversation
Please note that I couldn't test the change as the tests don't run for me on master. Happy to iterate based on your feedback, @marvinhagemeister. 🙂 |
Thanks to Travis, I see that removing The other problem is the same as on master:
Is there a use case for passing a function (or another object) as a child to a component? Since it's likely a mistake by the programmer, I would prefer to adapt the test instead of the type declaration. On the other hand, one could argue that it's up to the component what it does with its children (including whether to evaluate them with a function call). What do you think, shall I add |
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.
That's awesome, thanks for making a PR 🎉 The tests are failing because type object
is needed to get JSX children to work properly with TS.
A simple test case for boolean
would be the icing on the cake. Something like this would be amazing to have it in VNode-test.tsx
:
class BooleanComponent extends Component {
render() {
return false;
}
}
The tests can be run with npm run test:ts
. If it still doesn't work I'm happy to help debug further 👍
@KasparEtter Passing a function as a child is a valid pattern and the way the new Context-API in React 16 works https://reactjs.org/docs/context.html#consumer (we don't support it yet in preact). |
fee2473
to
b03b7ea
Compare
I updated my PR (by force-pushing an amended commit, I hope you don't mind that I squashed already but it was a tiny commit anyway). I hope you agree that the types belong to Two more observation based on React's types:
|
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 for making the PR even better 🎉 👍 👍
Fixes #1215.