fix(typings): allow to have primitives as children for built-in #369
Conversation
There was a problem hiding this comment.
Looks good to me, thank you for this contribution to the typings.
I'd like to look at the react node typing to see if this may result in other unexpected behaviour with css properties. I should get a chance to do this tomorrow morning. (~12hours).
@Ailrun has done some great work recently with these typings and may know.
| @@ -218,14 +218,6 @@ glamorous('div', { | |||
| withProps: '' | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
I'm assuming this was removed by accident as it looks like it should fail and appears unrelated to the changes. Would you mind re adding if that's the case thanks.
There was a problem hiding this comment.
This is removed on purpose, as this relaxation is the side-effect of the proposed change.
The new indexed type allows the value to be boolean, which makes the primaryColor: props.visible be valid.
Of course, it’s silly that we have this effect, but as noted in the other comment, it’d be awesome to not even have children in the CSSProperties. Until then this is a workaround.
The PR is effectively relaxing the type-checking for this instance, meaning fewer errors will be caught by TypeScript compiler. However, requirements that are too lax are much better than requirements that are too strict. In the latter case the developers fall into habit into silencing errors (with as any), defeating the entire purpose of typechecking in the first place.
| @@ -1926,7 +1926,7 @@ export interface CSSPropertiesLossy { | |||
| | undefined | |||
| | Array<CSSPropertiesComplete[keyof CSSPropertiesComplete]> | |||
| | CSSPropertiesLossy | |||
| | React.ReactChild | |||
| | React.ReactNode | |||
There was a problem hiding this comment.
Thanks for the update, I'd love to remove this entirely from this file as a ReactNode/ReactChild is not a css property.
But keeping things working in the meantime sounds great 💯.
There was a problem hiding this comment.
Me too, but small steps at a time. :-(
I have tried investigating the root cause, but I can not understand why does children match the index signature on CSSProperties rather than the existing children on React components. I tried overriding children even directly in CSSPropertiesLossy, but the index still takes priority. By the way, if I remove CSSPropertiesLossy entirely, my code typechecks. But it, of course, might not be the solution.
Fixup 08cfb31. It is valid to use `null` or `false` in react children, but the previous typings did not handle that case. `ReactChild` is a subset of `ReactNode`: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/98f1d85679fc581283cf6cc4d48ce8ae8b18e4a5/types/react/index.d.ts#L170 The commit is without tests for SVG, same as the commit being fixed.
|
Hey @denis-sokolov, sorry for the delay in looking at this. Really appreciate your work on this pr and following up on the issue. I pulled this down for a play, and ended up creating a pr which solves the issue in a different way (removing the need for https://github.com/paypal/glamorous/pull/372/files#diff-b6bac500e3d114b9016569ad1059fdd7 |
|
Seeing how #372 is delayed, perhaps we could merge this in the meantime? (The build error is trivially fixable) |
|
@denis-sokolov I will merge #372 as soon as @luke-john updates snapshot. Sorry for delaying. |
|
#372 is merged. IMO, we could close this. |
What: Incorrect typing for built-in components.
Why: Because today in client code I have to add a manual typing override:
<Div>{foo as any}</Div>iffoocan benull.How: By using the correct type from React library.
Checklist:
Fixup 08cfb31.
It is valid to use
nullorfalsein react children,but the previous typings did not handle that case.
ReactChildis a subset ofReactNode:https://github.com/DefinitelyTyped/DefinitelyTyped/blob/98f1d85679fc581283cf6cc4d48ce8ae8b18e4a5/types/react/index.d.ts#L170
The commit is without tests for SVG, same as the commit being fixed.