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

Fix Flow type for withTheme #1397

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

SimeonC
Copy link
Contributor

@SimeonC SimeonC commented Jun 12, 2019

What: Flow Typeing for withTheme isn't correctly altering props.

Why: As mentioned in comment you can't use withTheme easily with flow prop types.

How: Switching the React.ComponentType to React.AbstractComponent. To do this also changed it to a normal function as that displays the flow types in a more consise and clear way.

Checklist:

  • Documentation N/A
  • Tests N/A
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Jun 12, 2019

🦋 Changeset is good to go

Latest commit: 0708342

We got this.

Not sure what this means? Click here to learn what changesets are.

@SimeonC SimeonC force-pushed the fix-emotion-themeing-flow-type branch from 3f3b536 to e45175d Compare June 12, 2019 09:43
@SimeonC
Copy link
Contributor Author

SimeonC commented Jun 12, 2019

So there is a flow error present, and I'm not sure how to fix this as it's apparently a valid error according to the flow issue: facebook/flow#7612

It looks like this kind of error is due to the hoistNonReactStatics not supporting AbstractComponent.
Not sure how I can deal with this error...

EDIT: Doing some more thinking - I could just disable flow on that line. This may only cause issues if a flow user is relying on the hoisted statics.

@emmatown
Copy link
Member

I'd be happy with a $FlowFixMe.

@SimeonC SimeonC force-pushed the fix-emotion-themeing-flow-type branch from e45175d to 0708342 Compare June 12, 2019 11:14
@SimeonC
Copy link
Contributor Author

SimeonC commented Jun 12, 2019

OK, thanks for the confirmation. I've pushed up a fix with the $FlowFixMe included.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

@emmatown emmatown merged commit 3380373 into emotion-js:master Jun 12, 2019
@@ -30,6 +30,7 @@ test(`withTheme(Comp) hoists non-react static class properties`, () => {
const ComponentWithTheme = withTheme(ExampleComponent)

expect(ComponentWithTheme.displayName).toBe('WithTheme(foo)')
// $FlowFixMe
Copy link
Member

Choose a reason for hiding this comment

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

it would have made sense to add an explanation of why this is needed, for future reference.

I guess it's too late.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'tis never too late! Made a new PR #1399

SimeonC added a commit to SimeonC/emotion that referenced this pull request Jun 13, 2019
@SimeonC SimeonC deleted the fix-emotion-themeing-flow-type branch June 13, 2019 10:20
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants