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

Add correct PersistGate defaultProps to support newer flow versions #974

Merged
merged 3 commits into from
Jan 26, 2019

Conversation

erksch
Copy link
Contributor

@erksch erksch commented Jan 23, 2019

As mentioned in #953 there are type errors with some flow versions in the PersistGate component.

The problem with flow is described here:
facebook/flow#1660

A possible solution is described later in the issue and was already proposed by the creator of issue #953.

So what I did is adding children: null to the defaultProps and making loading and children not optional anymore.

@erksch erksch mentioned this pull request Jan 23, 2019
@kareemsalah227
Copy link

I only have a quick comment, correct me if I'm wrong please.

You can provide a loading component or you can ignore it, so it's kind of "optional", I think this might be the reason it was set optional in the flow type? If that is the case, is there another solution to make it optional and get rid of the flow error? (maybe by providing a default value or something).

Please let me know what do you think.

@erksch
Copy link
Contributor Author

erksch commented Jan 23, 2019

Removing the ? is okay here because flow will mark them as optional automatically when defaultProps are defined. You can read about it here: facebook/flow#1660 (comment)

@erksch
Copy link
Contributor Author

erksch commented Jan 23, 2019

And there seems to be no other option than removing the optional flag :/

@kareemsalah227
Copy link

Actually there is, I'll create a quick PR for that...

@kareemsalah227
Copy link

Ops, looks like I don't have permissions to push a branch to the repo, what should I do? @erksch

@kareemsalah227
Copy link

Nevermind, I've never contributed to an open source repo before, pardon my ignorance.
I forked the repo and created a cross-repo PR.
#975

There's a decent description in there, let me know what do you think @erksch .

@erksch
Copy link
Contributor Author

erksch commented Jan 25, 2019

@aguynamedben Hey what's your status? I'd like to get this merged. I think many people can not update their flow version properly if this is not fixed.

@aguynamedben aguynamedben merged commit 8a96ffc into rt2zz:master Jan 26, 2019
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