test: Flow definitions #97
Conversation
DianaSuvorova
changed the title
| ? $theme.borders.radius300 | ||
| : '0px', | ||
| boxShadow: $theme.lighting.shadow600, | ||
| backgroundColor: $theme && $theme.colors.background, |
There was a problem hiding this comment.
React context props are not enforceable So it has to be conditional. I wish we could use optional chaining but storybook doesn't support babel 7 yet.
| @@ -39,9 +44,7 @@ test('Helpers - Overrides - getOverrideProps', () => { | |||
| }); | |||
|
|
|||
| test('Helpers - Overrides - toObjectOverride', () => { | |||
| const CustomComponent = jest.fn(); | |||
| expect(toObjectOverride()).toBeUndefined(); | |||
There was a problem hiding this comment.
based on flow types of this fnc it takes override: OverrideT<T> as an argument. So can't be called with null or undefined. We need to either remove tests or update types.
| @@ -150,7 +150,8 @@ export type SharedStylePropsArgT = { | |||
| }; | |||
|
|
|||
| export type SharedStylePropsT = SharedStylePropsArgT & { | |||
| $theme: ThemeT, | |||
| $theme?: ThemeT, | |||
There was a problem hiding this comment.
$theme should not be optional here
There was a problem hiding this comment.
I think it is coming from context. If it is then:
I mentioned this above: React context props are not enforceable So it needs to be optional.
Do you see any other alternatives?
There was a problem hiding this comment.
At the level of a styled component $theme comes from props.
There was a problem hiding this comment.
Yeah even if context props are not enforceable, it seems like we can guarantee that our style functions will always receive a theme (if user doesn't specify a theme provider their page would be broken anyway). So ideally we should still type our style functions having $theme as required so we don't need to have null checks everywhere.
| @@ -17,9 +19,20 @@ export function getComponent<T>( | |||
| defaultComponent: React.ComponentType<T>, | |||
| ): React.ComponentType<T> { | |||
| if (override && typeof override === 'object') { | |||
| return override.component || defaultComponent; | |||
| if ( | |||
| typeof override.component === 'object' && | |||
There was a problem hiding this comment.
Why do we need this check here? Also, a component override would be a function type.
| (override.component instanceof React.StatelessFunctionalComponent || | ||
| override.component instanceof React.Component) | ||
| ) { | ||
| return override.component; |
There was a problem hiding this comment.
Even though these are used to refine the type, won't these still be called at runtime? override.component should either be a function or a class, it's not actually an instance of either of these things. And React.StatelessFunctionalComponent is not a actually a thing you can use in instanceof, it's a type, hence why unit-tests fail on this PR:
TypeError: Right-hand side of 'instanceof' is not an object
28 | }
29 | } else if (
> 30 | override instanceof React.StatelessFunctionalComponent ||
| ^
31 | override instanceof React.Component
32 | ) {
33 | return override;
I'm pretty sure we actually uncovered a bug in flow with this function that makes it difficult to type correctly, see facebook/flow#6666
I'll play around with some alternatives here.
This is going to fail there is 12 more flow errors to fix.
All of them are related to usage of
getComponent.@nadiia , @schnerd
Sometimes it is called with
nullas second argument - if this appropriate usage ofgetComponentfunction? if so we need to update typing of it.Sometimes styletron adds to confusion - I will look into those cases.