Skip to content
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

[flow] Fix Higher-order Component typing #8419

Merged
merged 40 commits into from
Sep 27, 2017

Conversation

rosskevin
Copy link
Member

@rosskevin rosskevin commented Sep 27, 2017

So this one is huge.

Essentially, I mis-typed the HOCs, which was hiding errors for static analysis. Once I corrected the HOC types based on the docs, I then ran into a flow issue where it did not recognize defaultProps inside the wrapped component. Luckily, there is a published type in react-flow-types that solves it.

The remaining fallout was immense. Over 1700 flow errors. I have fixed all the errors, and there should be no functional changes except one: withTheme. In the interest of consistency (and these HOCs are not easy to type), I altered the execution of withTheme from withTheme(Component) to withTheme()(Component) to match everything else we have and the same pattern used in recompose.

For release notes, it isn't necessarily a breaking change, but this PR is bound to reveal existing flow errors in the user's code which was previously hidden.

Also:

  • updated babel-plugin-flow-react-prop-types
  • removed remaining $FlowFix Props|State from 0.53 conversion
  • flow typed every remaining src tree component! We are 100%.

Breaking change

-  withTheme,
+  withTheme(),

@rosskevin rosskevin added bug 🐛 Something doesn't work flow breaking change labels Sep 27, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will hold future changes on the src. I'm gonna focus on upgrading enzyme.

// Provide the theme object as a property to the input component.
export default function withTheme<BaseProps: {}>(BaseComponent: ComponentType<BaseProps>) {
const factory = createEagerFactory(BaseComponent);
const withTheme = (): HigherOrderComponent<{}, InjectedProps> => (Component: any): any => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Why changing the API?

Copy link
Member

@oliviertassinari oliviertassinari Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unlikely that we will ever need configuration options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency. Sanity. My sanity. Seriously. If you want to edit it back, you can, but I'd just prefer one chosen HOC pattern.

Copy link
Member

@oliviertassinari oliviertassinari Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @istarkov could give us some light here. For instance, I can see the pure typing.

https://github.com/acdlite/recompose/blob/6064554ccceb07d126f9df61d4d6221c5fc726ea/types/flow-typed/recompose_v0.24.x/flow_v0.55.x-/recompose_v0.24.x.js#L138

@rosskevin Are you saying that we need HigherOrderComponent to get flow going through the HOCs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rosskevin recompose seem to have HOC factories as well as HOCs depending on the use cases. I will work on this if you prefer 👼 .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HigherOrderComponent solves the default prop not found issue linked in the description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, for most enhancers to type them properly they must be a function, pure is bad example as it just do nothing. So based on my experience there are no other ways

@rosskevin
Copy link
Member Author

I'm back, working on the karma tests now.

@rosskevin rosskevin merged commit 6f815c5 into mui:v1-beta Sep 27, 2017
@rosskevin rosskevin deleted the flow-react-flow-types-hoc branch September 27, 2017 22:10
@oliviertassinari oliviertassinari changed the title [flow] fix HOC typing [flow] Fix HOC typing Sep 30, 2017
@oliviertassinari oliviertassinari changed the title [flow] Fix HOC typing [flow] Fix Higher-order Component typing Sep 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants