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.

Sign up for GitHub

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

Jump to bottom

Add Flow types required for RN 0.59 migration #3442

Merged
merged 10 commits into from
Apr 26, 2019
Merged

Add Flow types required for RN 0.59 migration #3442

merged 10 commits into from
Apr 26, 2019

Conversation

borisyankov
Copy link
Contributor

This PR adds Flow types that if missing, result in errors in the React Native 0.59 upgrade PR (not because of RN but because of Flow 0.92)

Can be merged separately. Prerequisite for #3422

@borisyankov
Copy link
Contributor Author

@gnprice this should be ready to merge independently of RN 0.59

@gnprice
Copy link
Member

gnprice commented Apr 3, 2019

Thanks @borisyankov ! These look like good improvements in general, in addition to being required by the newer Flow 😄

Small comments:

  • Some lint errors; should fix.
  • Several of these are writing down a type that comes from RN upstream. I think it's important for the reader to see what bit of upstream API they're intended to correspond to and what source you were consulting to write them -- both to understand the code, and also to make it easy to reproduce your work to see if e.g. the upstream API has changed a year from now, and apply updates.
    • Each change already provides this information quite nicely in the commit messages -- thanks! Particularly with the links to upstream docs. This is a kind of information that I think actually belongs in the tree itself, in jsdoc -- it's important information even for understanding the new code as it is, not only understanding how it compares to the old code. Would you move it in there?
    • The ConnectionType is one that does this well already. (Except I guess a smaller nit: since the link is there in the comment, I'd leave it out of the commit message.)

Not a blocker to merge, but a followup-thought that this brought to mind: where we're writing types that really belong to RN, or to another third-party library, the ideal place for them would actually be in a libdef, rather than inside our app code. We generally haven't done that because we get most of our libdefs from the flow-typed command/repo, and we haven't developed a workflow for making changes to that, so there's a lot of friction. I think it may not actually be hard to start doing it, though; I might sit down and try it sometime.

@borisyankov borisyankov force-pushed the flow-pre-rn59 branch 3 times, most recently from a0a61cd to 8a08d8e Compare April 4, 2019 18:42
@gnprice gnprice mentioned this pull request Apr 15, 2019
Closed
@gnprice
Copy link
Member

gnprice commented Apr 17, 2019

Thanks again @borisyankov !

You mentioned this PR in an audio conversation the other day, so I came back to look at it. It looks like you've resolved the lint errors; but my other comment is still outstanding:

Several of these are writing down a type that comes from RN upstream. I think it's important for the reader to see what bit of upstream API they're intended to correspond to and what source you were consulting to write them [...]

Would you please address that? Looking forward to getting to merge this 🙂

@gnprice
Copy link
Member

gnprice commented Apr 17, 2019

Also, I'll just add a link here to make this PR discoverable directly from the issue: this is part of #3399 .

@borisyankov borisyankov force-pushed the flow-pre-rn59 branch 3 times, most recently from 0377fbc to b087730 Compare April 18, 2019 23:09
@borisyankov
Copy link
Contributor Author

This is ready for review and merge, separately from the RN 0.59 PR.

The changes are mostly minor commit message improvements but also this commit is new:
UserItem and related components: onPress now takes just 'email'

@gnprice
Copy link
Member

gnprice commented Apr 26, 2019

Thanks for these revisions. The new commit looks great.

In the places I requested changes, I'm disappointed because it looks as if you didn't really read what I said or make much effort to respond. This matters to me because it means I feel like I wasn't heard. I'll follow up with you separately.

In this instance, the requested changes are pretty straightforward to make; so I'll go ahead and merge, perhaps with some adjustments.

borisyankov and others added 10 commits April 26, 2019 16:02
@borisyankov @gnprice
UserItem: Refactor onPress to take just email.
edf3c00 
`UserItem` component and several related ones, when using an
`onPress` callback, were passing an object consisting of `email`
and `fullName` of the user being selected, and a variety of
callbacks used one or the other.

Trying to type this more precisely and 'strict'-ly revealed that
we are treating this object rather loosely.  The `fullName` info
was almost never used.  So, we omit it.

This is a pure refactor that removes the `fullName` information
and keeps the `email` as a string instead of property of an object.

Only `PeopleAutocomplete` is slightly reworked to get the `fullName`
via the `email` value, a trivial `find` in an array of users.
@borisyankov @gnprice
flow: Add types to handleAppStateChange
d782a8b 
This does not exist as a Flow type in React Native, so we can't
import it. But we can just define it ourselves.

Documentation at:
https://facebook.github.io/react-native/docs/appstate
@borisyankov @gnprice
flow: Add ConnectionType and use it in event handler
2b745a9 
While documented, this type is not explicitly defined in the RN's
source files. So we define it locally and reuse.

Documentation:
https://facebook.github.io/react-native/docs/netinfo
@gnprice
flow: Move ConnectionInfo type to narrow scope, and match name.
d861a79 
The linked docs page uses the name `ConnectionType` specifically for
the enum found in the `type` property of this type.  The other
property's type is `EffectiveConnectionType`.  The object type
containing both isn't given a name, but parameters of that type are
called `connectionInfo`, which seems apt.

So, avoid reusing the upstream docs' name `ConnectionType` for
something related but different, which risks being quite confusing.

Also move the type from the global `types.js` to the one place where
we refer to it, so there isn't such a general name floating around in
basically our global namespace; and add a TODO to move it to a still
more appropriate spot, namely a libdef.
@borisyankov @gnprice
flow: Add 'Linking'-related types in 'AuthScreen'
693e288 
The `LinkingEvent` type does not exist explicitly in React Native's
code and was confirmed correct by reading the source code and looking
at the documentation in:
https://facebook.github.io/react-native/docs/linking

`getInitialURL` returns only `string` or `null` but is already
internally defined as `?string` in RN so use that instead.
@gnprice
flow: Put in jsdoc some helpful info from recent commits.
b4b1929 
This is information that was in the commit messages of the commits
that added these bits of code.  It's useful information, and important
not only for understanding the change (i.e. for comparing old and new
versions of the code), but instead mainly for understanding the new
code as it is.  So, the best place for it is in comments on the code
itself, right in the tree.
@borisyankov @gnprice
flow: Add types in languages.js.
1b66748 
Add type for the object we use for language meta-data and
a type for the exported value based on it.
@borisyankov @gnprice
flow: Add types to 'LanguagePicker'
723123e 
Using the newly typed exports in 'languages.js' add types to the
code that uses that data.
@borisyankov @gnprice
flow: Add 'LayoutEvent' type to 'ComposeBox'
c6f2ac3 
Import `LayoutEvent` from the semi-obscure place it is defined
and use it to type the layout event handler.
@borisyankov @gnprice
flow: Add misc missing types
fb136e4 
Adds very minor types to several files that would result in errors
in upcoming Flow versions.
@gnprice
Copy link
Member

gnprice commented Apr 26, 2019

And merged! Thanks again.

@gnprice gnprice merged commit fb136e4 into zulip:master Apr 26, 2019
gnprice added a commit to gnprice/zulip-mobile that referenced this pull request Jul 25, 2019
@gnprice
Upgrade React Native to v0.59, from v0.57.
dfbdd97 
Fixes zulip#3399!

The bulk of the work of this upgrade went into a large number of
commits previous to this one.  The majority of the last few dozen
commits, after 1c86488^, were part of the upgrade; some discussion
in zulip#3561.  An earlier wave of effort focused on getting things working
with the upgrade of Flow to v0.92; see zulip#3450 and especially f8c9810,
and then zulip#3453 and zulip#3442.  Some other early discussion is at zulip#3422.

This commit itself was done mainly by running:

  $ tools/upgrade rn v0.59.10

Then, that tool needs a bit of an update for changes upstream.
Because I'm feeling pressed to get this upgrade out the door (and
to deal with the various separate trouble on iOS), I just took
care of the other relevant packages by hand: updated `@babel/core`
and `metro-react-native-babel-preset` in package.json according to
the diff seen in `rn-diff-purge`, then reran `yarn`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@borisyankov @gnprice