[BPK-2295] Upgrade React Native to 0.59.0 #77

shaundon · 2019-03-12T18:56:47Z

Tasks

Upgrade RN
Run on iOS
Run on Android
Confirm tests are ok
Upgrade Flow to align with RN 0.59.0's version
Go through each story and note anything broken (iOS)
Go through each story and note anything broken (Android)
Upgrade the React and RN versions in the package.json of each component

Issues identified so far

Jest doesn't run (fixed by adding testMatch: null)
Linear gradient doesn't work (fixed by upgrading to latest)
Flow issues (legit errors, fixed manually)
Calendar is redboxing on iOS (fixed by enforcing numbers instead of dates sent over the bridge)
Async storage warning (this is an issue with Storybook, I've made a PR). I think this is acceptable as-is - it just displays a yellow box error when Storybook first loads and I anticipate the fix being merged and released soon.
Doesn't run on Android (> Could not get unknown property 'mergeResourcesProvider' for object of type com.android.build.gradle.internal.api.ApplicationVariantImpl.) (Fixed by doing this Could not get unknown property 'mergeResourcesProvider' for object of type com.android.build.gradle.internal.api.ApplicationVariantImpl. wix/react-native-navigation#4757 (comment))

shaundon · 2019-03-13T07:58:09Z

ios/native.xcodeproj/project.pbxproj

@@ -202,6 +205,7 @@
 		D674D18120BEDBCA008F2E97 /* Frameworks */ = {
 			isa = PBXGroup;
 			children = (
+				60005AA122382F040076B576 /* JavaScriptCore.framework */,


I had to manually link JavaScriptCore as mentioned in the 0.58.0 release notes:

if you are an iOS developer, you'll need to manually link JavaScriptCore.framework when upgrading;

shaundon · 2019-03-13T08:02:23Z

package.json

+    "react": "16.6.3",
+    "react-dom": "16.6.3",
+    "react-native": "0.58.5",
+    "react-native-linear-gradient": "^2.5.3",


ColorPropType was deprecated in 0.58 so I upgraded react-native-linear-gradient to the latest version, which seems to fix the problem, and gets us back on a normal version and not this branch 🙌

shaundon · 2019-03-13T08:40:10Z

packages/react-native-bpk-component-image/src/BpkImage.js

@@ -56,6 +59,7 @@ const styles = StyleSheet.create({

 export type Props = {
  ...$Exact<ImageProps>,
+  style: ViewStyleProp,


Flow was giving us an error here because Image.style isn't quite the same as View.style, but we were using Image's style type here then applying it to a view (on line 144). So, I've made it explicitly use the style type from View.

backpackbot · 2019-03-13T08:50:03Z

	Warnings
⚠️	One or more packages have changed, but `UNRELEASED.md` wasn't updated.
⚠️	`package-lock.json` was updated. Ensure that this was intentional.

Generated by 🚫 dangerJS against 5e69dad

shaundon · 2019-03-14T10:22:59Z

.flowconfig

@@ -24,7 +24,6 @@
 [libs]
 node_modules/react-native/Libraries/react-native/react-native-interface.js
 node_modules/react-native/flow/
-node_modules/react-native/flow-github/


Removed to fix this notice:

Can we also have a look at the .flowconfig of RN at 0.59.0? That might contain some new configuration that we should also add.

I was going to do that, but if the config we have is already working for us, what is there to be gained by changing it?

We used that mentality previously, only to find out that we were missing out on a bunch of flow functionality.

In other words, when we aligned our version of flow to RN's version and subsequently aligned the .flowconfig, we found errors we were otherwise oblivious to.

Who knows, perhaps #77 (comment) goes away when we align the config?

That's a fair point! Let me investigate 🤓

I've updated it based on RN's latest configs, no changes it seems (including #77 (comment) unfortunately) but as you say, good to be aligned with the latest and greatest.

shaundon · 2019-03-14T10:24:57Z

package.json

@@ -49,6 +49,7 @@
    "preset": "react-native",
    "verbose": true,
    "testRegex": ".*-test(\\.ios)?(\\.android)?\\.js$",
+    "testMatch": null,


Fixes a Jest issue:

kulshekhar/ts-jest#756 (comment)

shaundon · 2019-03-14T10:25:57Z

package.json

-    "babel-plugin-jest-hoist": "^23.2.0",
+    "@storybook/react-native": "^4.1.14",
+    "babel-jest": "^24.5.0",
+    "babel-plugin-jest-hoist": "^24.3.0",


I updated these to try and fix another issue but the upgrade doesn't seem to break anything so may as well do it.

shaundon · 2019-03-14T10:26:13Z

package.json

    "bpk-tokens": "^27.4.7",
    "colors": "^1.3.3",
    "danger": "^7.0.11",
    "eslint-config-skyscanner-with-prettier": "^0.8.0",
    "eslint-plugin-flowtype": "^3.2.0",
    "eslint_d": "^7.2.0",
-    "flow-bin": "^0.78.0",
+    "flow-bin": "^0.92.0",


To align with RN 0.59's Flow version.

Nitpick: maybe this should explicitly be "0.92.0".

Fully appreciate that I probably set the carat last time haha

Haha - well I went with the carat because that's what RN does in their repo but I'm happy to lock it if you feel strongly about it.

No lets follow them :)

shaundon · 2019-03-14T12:11:23Z

android/build.gradle

@@ -9,7 +9,7 @@ buildscript {
        mavenCentral()
    }
    dependencies {
-        classpath 'com.android.tools.build:gradle:3.2.0'
+        classpath 'com.android.tools.build:gradle:3.3.2'


To fix a build issue (see wix/react-native-navigation#4757 (comment))

shaundon · 2019-03-14T12:12:01Z

package.json

  },
  "greenkeeper": {
    "ignore": [
      "flow-bin",
      "react",
      "react-dom",
      "react-test-renderer",
-      "react-native",
-      "react-native-linear-gradient"
+      "react-native"


We no longer need to ignore linear gradient.

shaundon · 2019-03-14T12:13:55Z

packages/react-native-bpk-component-calendar/src/NativeCalendar.ios.js

@@ -59,7 +70,7 @@ class BpkCalendar extends Component<Props, {}> {
      const handle = findNodeHandle(this.calendarRef.current);
      UIManager.dispatchViewManagerCommand(
        handle,
-        UIManager.RCTBPKCalendar.Commands.forceRender,
+        UIManager.getViewManagerConfig('RCTBPKCalendar').Commands.forceRender,


To remove a deprecation warning. See https://github.com/facebook/react-native/blob/d6236796b285e6ad19c53c5308a0ad9c10792a05/Libraries/ReactNative/UIManagerProperties.js#L15

shaundon · 2019-03-14T12:15:45Z

packages/react-native-bpk-component-calendar/src/NativeCalendar.ios.js

        onDateSelection={onChangeSelectedDates}
-        selectedDates={selectedDates}
+        selectedDates={selectedDates.map(parseDateToNative)}


This was a nightmare issue to fix. Essentially something appears to have changed on the RN iOS side that means dates sent over the bridge weren't making it over properly, so it was failing to parse and causing red box errors.

I never figured out what exactly changed internally to make this happen, but I fixed it by taking inspiration from the Android version of this component and cast everything to a timestamp before sending it over the bridge, which makes it work.

shaundon · 2019-03-14T12:17:02Z

...ar/src/android/src/main/java/net/skyscanner/backpack/reactnative/calendar/CalendarPackage.kt

    return mutableListOf()
  }

-  override fun createViewManagers(reactContext: ReactApplicationContext?): MutableList<ViewManager<*, *>> {
+  override fun createViewManagers(reactContext: ReactApplicationContext): MutableList<ViewManager<*, *>> {


After upgrading Gradly things I started seeing build errors here that we were failing to override the methods we were supposed to. I checked and it appears the signatures changed so that ReactApplicationContext is no longer optional. This change doesn't appear to break anything.

shaundon · 2019-03-14T12:17:47Z

packages/react-native-bpk-theming/stories.js

-      themeId: value,
-      theme: this.themes[value],
-    });
+    if (typeof value === 'string') {


Ridiculous check to stop Flow mithering me

georgegillams · 2019-03-14T14:06:21Z

packages/react-native-bpk-component-button-link/src/common-types.js

@@ -21,6 +21,7 @@
 import { type Node } from 'react';
 import PropTypes from 'prop-types';
 import { makeThemePropType } from 'react-native-bpk-theming';
+import type { PressEvent } from 'react-native/Libraries/Types/CoreEventTypes';


Are you meant to use stuff like this? Isn't there a danger that they change what's exposed as it's not part of the official API?

There is that risk, but I don't see another way of doing it as it doesn't appear to be exported on the public interface, and if it breaks we'll find out as soon as we try to upgrade RN and can change it.

I did try this:

type TouchableProps = ElementProps<typeof TouchableWithoutFeedback>; type TouchableOnPressProp = $PropertyType<TouchableProps, 'onPress'>;

But I get a Flow error and I couldn't figure it out:

Cannot instantiate $PropertyType because property onPress is missing in mixed [1]. 29│ } from './customPropTypes'; 30│ 31│ type TouchableProps = ElementProps<typeof TouchableWithoutFeedback>; [1] 32│ type TouchableOnPressProp = $PropertyType<TouchableProps, 'onPress'>;

@matthewdavidson may have an opinion as he's done similar things in his Flow-ing PRs.

You can use SyntheticEvent<>, no?

It doesn't like that anymore, that's what we were using before this PR:

matthewdavidson

LGTM. I manually tested each component across iOS & Android. Found one issue with the RN Switch component that Shaun already fixed (see facebook/react-native@d6ee448#diff-bb87898c3a651ca8f18771f770e8a808) but it'll have to wait until 59.2 to land.

shaundon added the I WIP my hair back and forth label Mar 12, 2019

shaundon marked this pull request as ready for review March 12, 2019 19:01

shaundon commented Mar 13, 2019

View reviewed changes

shaundon added 4 commits March 13, 2019 16:55

[BPK-2295] Upgrade React Native to 0.58.5

5926c88

[squash] Upgrade Flow and fix errors

61b3cc7

[squash] Fix Android Flow errors

f3dae33

[squash] Update React and RN version in components

982b89c

shaundon force-pushed the BPK-2295-upgrade-rn branch from 63fde1f to 982b89c Compare March 13, 2019 17:16

[squash] Move to 0.59.0 and update tests

20c095b

shaundon changed the title ~~[BPK-2295] Upgrade React Native to 0.58.5~~ [BPK-2295] Upgrade React Native to 0.59.0 Mar 13, 2019

shaundon added 3 commits March 13, 2019 17:59

[squash] Fix Flow errors

056e8c5

[squash me] Bump package versions

0454877

[squash] Get Android working

8252171

shaundon commented Mar 14, 2019

View reviewed changes

shaundon added 2 commits March 14, 2019 10:35

[squash] Remove rn-linear-gradient from greenkeeper ignore

ea13941

[squash me] Enforce numbers for dates sent over the bridge on iOS

fe534cf

shaundon commented Mar 14, 2019

View reviewed changes

shaundon removed the I WIP my hair back and forth label Mar 14, 2019

[squash] Update calendar snapshots

6345652

shaundon requested a review from georgegillams March 14, 2019 13:30

georgegillams reviewed Mar 14, 2019

View reviewed changes

georgegillams previously approved these changes Mar 14, 2019

View reviewed changes

[squash] Update flow configs

6a690fd

shaundon dismissed georgegillams’s stale review via 6a690fd March 14, 2019 16:33

[squash] Remove unused import

5e69dad

matthewdavidson approved these changes Mar 18, 2019

View reviewed changes

shaundon merged commit d7fc4f6 into master Mar 18, 2019

shaundon deleted the BPK-2295-upgrade-rn branch March 18, 2019 09:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BPK-2295] Upgrade React Native to 0.59.0 #77

[BPK-2295] Upgrade React Native to 0.59.0 #77

shaundon commented Mar 12, 2019 •

edited

shaundon Mar 13, 2019

shaundon Mar 13, 2019

shaundon Mar 13, 2019

backpackbot commented Mar 13, 2019 •

edited

shaundon Mar 14, 2019

matthewdavidson Mar 14, 2019

shaundon Mar 14, 2019

matthewdavidson Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

matthewdavidson Mar 15, 2019

shaundon Mar 15, 2019

matthewdavidson Mar 15, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

shaundon Mar 14, 2019

georgegillams Mar 14, 2019

shaundon Mar 14, 2019

matthewdavidson Mar 14, 2019

shaundon Mar 14, 2019

matthewdavidson left a comment

[BPK-2295] Upgrade React Native to 0.59.0 #77

[BPK-2295] Upgrade React Native to 0.59.0 #77

Conversation

shaundon commented Mar 12, 2019 • edited

Tasks

Issues identified so far

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backpackbot commented Mar 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewdavidson left a comment

Choose a reason for hiding this comment

shaundon commented Mar 12, 2019 •

edited

backpackbot commented Mar 13, 2019 •

edited