search

chenglou / react-motion

A spring that solves your animation problems.
MIT License
21.71k stars 1.15k forks source link

Upgrade typings to flow 0.53.1 #485

Closed AugustinLF closed 7 years ago

AugustinLF commented 7 years ago

I simply ran the flow-upgrade command, and fixed the ReactElement types. I didn't change anything else, perhaps you'd like me to replace the import * as React (which is what flow-upgrade does) by more granular imports?

chenglou commented 7 years ago

Oops, sorry I didn't see this one and merged #486 first... Can you check if that PR is suitable?

AugustinLF commented 7 years ago

The PR was correct, but didn't take into account the React.Node type, which I believe is more suited for the return value of the children function, so I rebased on top of master.

chenglou commented 7 years ago

Sounds fair. Thanks!

chenglou commented 7 years ago

Wait, no, React.Children.only returns a ReactElement: https://github.com/facebook/react/blob/cd52f7ed80e23b569634d34e42f8a6ae5948444b/src/isomorphic/children/onlyChild.js#L28

I'll revert this; if I'm wrong then I'll re-merge.

AugustinLF commented 7 years ago

Yep, you're right, my bad, didn't consider that React.Node is a React.ChildrenArray, so a children function with my definition could return [<div />, <div />], which is not compatible.

Sorry for the trouble!

chenglou commented 7 years ago

No worries, thanks for the PR still =)

dapetcu21 commented 7 years ago

The problem with the ReactElement typing is that it doesn't allow null, falsy or string values, which are still valid return values of render(). For example, in my code I have:

<Motion style={{ opacity: spring(someCondition ? 0 : 1) }}>
  {({ opacity }) => opacity !== 0 && (
    <SomeComponent />
  )}
</Motion>

Which fails the type check because it can return false.

AugustinLF commented 7 years ago

@dapetcu21 I created the PR (#497) to fix this, could you test your use cases against my fork (the branch is flow/onlyChildren)?

dapetcu21 commented 7 years ago

Cool! I'll do that, but I can't right now. I'll check it out on Wednesday when I get back to work

AugustinLF commented 7 years ago

@dapetcu21 any update on that?

dapetcu21 commented 7 years ago

Oh. Sorry. I forgot about this. I'll add a reminder for tomorrow.

dapetcu21 commented 7 years ago

@AugustinLF Yup. I can confirm #497 solves my issue. Sorry for taking this long. Things came up and I forgot about this issue.