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

Upgrade to React Native 0.57 #2955

Closed
wants to merge 32 commits into from
Closed

Conversation

borisyankov
Copy link
Contributor

@borisyankov borisyankov commented Sep 12, 2018

Based on RN 0.56 PR

Make sure to update to latest Android JDK.

@borisyankov
Copy link
Contributor Author

@gnprice This PR is ready.

@borisyankov borisyankov force-pushed the rn57 branch 3 times, most recently from 1334409 to 3fc8aa1 Compare September 23, 2018 19:38
@borisyankov borisyankov changed the title WIP: Upgrade to React Native 0.57 Upgrade to React Native 0.57 Sep 26, 2018
@borisyankov
Copy link
Contributor Author

FYI: Just removed the WIP from the name of the PR.

@gnprice
Copy link
Member

gnprice commented Sep 26, 2018

OK. Let's focus on getting the 0.56 upgrade completely in (so #2789, and I think that means #2994 first), before turning much attention to this again -- that will help some other ongoing work, and it'll also reduce the amount of stuff for us to keep track of in this PR.

Resolves zulip#2788

Upgrade dependencies and Flow version
The result of running `flow-typed update`.

Updates to the latest annotations potentially considering the
new Flow version.
Configure Jest to use a custom transformer coming with React Native.
facebook/react-native#19859 (comment)
Flow was not able to choose from two versions of the function:
 * ((...formattedMessage: Array<React$Node>) => React$Node)
 * (string => React$Node)

Giving the concrete type fixes this confusion.
Ignores a rather complicated Flow issue (Cannot instantiate React.Element ...)
that will likely be fixed with better `flow-typed` declarations in
near future.

Way too cryptic to fix, and it is not pointing to any actual issue
with our code. Ignore for now.
A rather strange issue arises from the `flow-typed` definitions
and the way Flow inferes the return type of the ZulipStatusBar
when wrapped with `connect`.

A rather long investigation and search for a concrete fix was futile.
There appears to be nothing wrong with our code and I suspect this
will be fixed either with a newer Flow version or with a change to
the type definitions in `flow-typed`.
Previously we didn't have to, but now it results in an error.
Do the right thing and import the type explicitly.
Flow is showing an error because of incomplete typing for
the DOM type StyleSheet, not being aware of `insertRule` function.

Added a $FlowFixMe for that.
Remove the $FlowFixMe comments as Flow no longer rises:
`Props has no `narrow` property`
Fixes Xcode project settings warnings.

This change introduced in the Starting project template here:
facebook/react-native@c298e0a#diff-065bc63a1a0283f31962d4ab2316087b
Bump gradle to 4.4, version used by android studio 3.x and gradle plugin 3.x. This will help make migration easier and smoother.

Upgraded in React Native here:
facebook/react-native@33d20da#diff-fd9d61e37d939c941e7a62077f38d62d
This was added when upgrading to RN 0.53
Now this configuration is moved to `metro-config` package, we no
longer need to support it.

Note: This config is messing up RN 0.57 so we need to remove it!
Upgrades the old Grade 2.x to 3.x config.

This is the exact `diff` that RN 0.57 implements.
The `compile` configuration is now deprecated. Recommended is the
usage of `implementation`.

More information about these here:
https://docs.gradle.org/current/userguide/java_library_plugin.html#example_declaring_api_and_implementation_dependencies
@borisyankov
Copy link
Contributor Author

The commits here are also ready to be reviewed.
I only thing that is not working OK is the SpinningProgress component that uses the ART library from the core react-native has issues rendering on Android. Confirmed with @jainkuniya that it does not work on his machine on a pristine 0.57 project too.
I will either figure out a fix, or replace the visual with something not using the ART lib.

(The currently latest version of 0.57.2 has issues)

Both updrade tools failed to run:
 * react-native-git-upgrade
 * react-native upgrade

This will be a manual but thorough old-school upgrade.
Not a problem, since we did that already for all but the last upgrade.

First step is just to upgrade the dependencies and the flow config file.

We also replace "babel-preset-react-native" with "metro-react-native-babel-preset"
and then .babelrc changes to reflect that.
The props we use to set the colors of the Switch component
`onTintColor` and `tintColor` are now deprecated.

Using the new `trackColor` property we set the exact same values
to preserve the exact style we had before.
Babel 7 is required to build the upcoming versions of React Native,
while still compatible with the current version.

This PR consists of:
1. Upgrade `babel-core`. The next version has changed names, so
use the different package.
2. Upgrade `babel-preset-react-native`
3. Change `generate-webview-js` script to use Babel 7.
This fixes a blocker-issue with RN 0.57.2 that prevents the
project from building on Android.

The original project is no longer supported, but a very active
fork is being maintaned and named as `rn-fetch-blob`.

This was manually tested and works fine. It contains multiple
fixes for issues that we do not have reports for, but feels nice
to auto-fix issues.
React Native's ART library is important and we probably will use
it more in the future. It is the only good option when we want to
achieve some visual elements / styles that we would use CSS or
SVG on the web instead.

Unfortunately, there is a serious bug on Android, that have existed
for a while. Surprisingly we did not encounter this bug until RN 0.57
but it has always been there.

Visually, the issue manifests as black spinning square instead of
the usual progress spinner.

Bug reported to React Native here:
facebook/react-native#17565

A possible solution looks something like:
macdoum1/react-native@5878481

Possible alternatives evaluated:
 * Styles (ala CSS) - previous implementation used that, but was not
reliable between versions (due to RN issues)
 * use third-party component - a bit too complicated, and they suffer
from the same issues. For example:
bartgryszko/react-native-circular-progress#105
 * use static images - that is the chosen workaround

The exact look was recreated in Sketch and exported as PNG. We use
two versions - one with 'brand' color and one 'black'. We are losing
the functionality to change 'thickness' which we don't use.

Once the bug in ART is fixed upstream, we will revert this change.
@borisyankov
Copy link
Contributor Author

All unmerged commits from here were appended to the 0.56 PR. Closing this one.

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.

2 participants