chore: fix flow types for overrides, popover #98
Conversation
| const Root = getOverride(RootOverride) || StyledRoot; | ||
| const Checkmark = getOverride(CheckmarkOverride) || StyledCheckmark; | ||
| const Label = getOverride(LabelOverride) || StyledLabel; | ||
| const Input = getOverride(InputOverride) || StyledInput; |
There was a problem hiding this comment.
Renamed getComponent to getOverride, removing the defaultComponent arg.
The return type of getComponent was ?React.ComponentType<T>, meaning that it could return a component or null/undefined. So even if we know that null/undefined can't be returned because we've passed a valid default, flow will still complain that we can't use <Checkmark because it may be null/undefined.
For this reason, i think we should just switch to using a getOverride(override) || DefaultComponent pattern to avoid having to do null checks on the output before we use them.
There was a problem hiding this comment.
I like this change 👍 The return was nullable because it actually was allowed to accept null as a second arg and return null as a result (used in Before and After components in Input).
| // TODO remove this 'any' once this flow issue is fixed: | ||
| // https://github.com/facebook/flow/issues/6666 | ||
| // eslint-disable-next-line flowtype/no-weak-types | ||
| return (override: any).component; |
There was a problem hiding this comment.
@vicapow said that this is probably the best solution for now
| function getMockComponent<T>(): React.ComponentType<T> { | ||
| const mock: React.ComponentType<T> = () => null; | ||
| return mock; | ||
| } |
There was a problem hiding this comment.
I still think that we should be comfortable with using const mock: any = jest.fn();, as we do in many other tests, however I'm not using any jest mock functionality in this file so this stricter type works.
| expect(toObjectOverride()).toBeUndefined(); | ||
| // $FlowFixMe - Calling toObjectOverride with null |
There was a problem hiding this comment.
We should be comfortable using $FlowFixMe in these cases. Not all customers will be using flow, so we may still want to test that our functions have certain behavior when passed bad input which doesn't match flow types.
There was a problem hiding this comment.
@schnerd why don't we be explicit with flow that the toObjectOverride can be called with no args or with null?
so toObjectOverride<T>(override?: ?OverrideT<T>): OverrideT<T>
There was a problem hiding this comment.
Because it's not intended to be called with undefined/null, and if customers are using flow we should define types that protect against that. This is a test designed to make sure the function does sane things for customers who aren't using flow and pass bad data.
| const Root = getOverride(RootOverride) || StyledRoot; | ||
| const Checkmark = getOverride(CheckmarkOverride) || StyledCheckmark; | ||
| const Label = getOverride(LabelOverride) || StyledLabel; | ||
| const Input = getOverride(InputOverride) || StyledInput; |
There was a problem hiding this comment.
I like this change 👍 The return was nullable because it actually was allowed to accept null as a second arg and return null as a result (used in Before and After components in Input).
| expect(toObjectOverride()).toBeUndefined(); | ||
| // $FlowFixMe - Calling toObjectOverride with null |
There was a problem hiding this comment.
@schnerd why don't we be explicit with flow that the toObjectOverride can be called with no args or with null?
so toObjectOverride<T>(override?: ?OverrideT<T>): OverrideT<T>
Building off Diana's work in #97, makes the necessary changes to get flow passing so we can export types from our package (#94).