Skip to content

Conversation

Ashoat
Copy link
Member

@Ashoat Ashoat commented Jul 7, 2017

react-native -> 0.46.1
react -> 16.0.0-alpha.12
flow-bin -> 0.47

Flow 0.47 has stronger opinions about passing parameters to functions that are defined not to accept them, so I had to fix several Flow errors that popped up. In some cases the type definition needed to be corrected, in some cases the functions didn't list parameters they could take, and in some cases extraneous parameters were being passed.

@Ashoat
Copy link
Member Author

Ashoat commented Jul 9, 2017

There was still a couple Flow errors that were popping up when using this react-navigation on a stock React Native 0.46 app, and they had to with unused suppressions in StackRouter-test.js. I removed those suppressions and identified they were only necessary because of a mismatch between the Jest version in package.json (v20) and the Flow typedef version for Jest (v17). I updated the typedef with flow-typed, as well as updating Jest to the latest version, and I was able to remove the suppression.

const subAction = route.index !== undefined && route.index !== 0 // if the child screen is a StackRouter then always navigate to its first screen (see #1914)
? NavigationActions.navigate({ routeName: route.routes[0].routeName })
: undefined;
let subAction = undefined;

This comment was marked as abuse.

This comment was marked as abuse.

@Ashoat
Copy link
Member Author

Ashoat commented Jul 13, 2017

Now that a new version has been released correcting the single Flow 0.47 error, I've updated the version of react-native-tab-view as well.

Would appreciate a review! @matthamil, @satya164?

@satya164
Copy link
Member

LGTM. cc @skevy

@Ashoat
Copy link
Member Author

Ashoat commented Jul 17, 2017

Can we get this merged soon please?

The failed tests reflect eslint errors that are outside the scope of this PR: flowtype/no-weak-types (which errors out on every any or Object usage) and prettier/prettier. I'm guessing somebody enabled these rules without making sure that the repo didn't have any errors.

@Ashoat
Copy link
Member Author

Ashoat commented Jul 21, 2017

Another ping here! Could somebody accept this PR please?

@Ashoat
Copy link
Member Author

Ashoat commented Jul 21, 2017

@skevy has independently implemented most of the changes in this PR, so I'm going to close it.

My goal with this PR was to help maintain a version of react-navigation that works with the latest RN release without any Flow errors. I think it's a pretty negative experience for a user today to get Flow errors when installing this package on a clean RN starter app.

Unfortunately, neither the latest version of the react-navigation npm package, nor what's in master on this repo, are currently able to evaluate with no Flow errors on a fresh copy of RN. The issues with the npm package is that it is far too old, and the issue with master is that it is using new Flow features that aren't compatible with the version of Flow RN 0.46 declares in its .flowconfig. react-navigation uses Flow 0.49, but RN 0.46 declares Flow 0.47 in its .flowconfig.

I am willing to try and help with this if @skevy sees this as a worthy goal, and is willing to review and accept PRs. That said, my assumption for now is going to be that you are tracking towards a new npm release that will be Flow-error-free in concert with a future version of react-native (hopefully RN 0.47, which I assume will have Flow 0.49).

@Ashoat Ashoat closed this Jul 21, 2017
@skovhus
Copy link

skovhus commented Sep 19, 2017

Good job @Ashoat! Would be so nice to get these errors fixed : )

But when looking at this now, this PR could be split into a few smaller PRs. Currently a commit like this 094a9cc is rather hard to review and combines to unrelated changes. Maintainers likes small and focused PRs.

Would really to see some new PRs for this and hopefully some of the new maintainers have time to review it.

@matthamil
Copy link
Member

I'm going to go back through this PR soon. In a quick pass, everything looks good. Assigning myself to this PR.

@matthamil matthamil self-assigned this Sep 19, 2017
@Ashoat
Copy link
Member Author

Ashoat commented Sep 19, 2017

Great to see some attention here!

@matthamil: This PR is unfortunately out-of-date, and would not apply cleanly to today's version of this repo, which is why I closed it. I can try to put up a new version later today, probably as a separate PR. I'd avoid reviewing until then.

@skovhus: Fair feedback. I'll see if I can break down my commits a bit better, though it's worth noting that the result of flow-typed update is always gonna be messy. It's probably best for a reviewer to ignore the flow-typed folder entirely.

@Ashoat
Copy link
Member Author

Ashoat commented Sep 20, 2017

Okay, #2619 is the new PR.

@Ashoat
Copy link
Member Author

Ashoat commented Sep 20, 2017

@matthamil, #2619 is ready for a look! I can't seem to assign reviews in this repo, so would be awesome if you could self-assign.

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.

4 participants