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

RN v0.61 upgrade followup. #4152

Merged
merged 6 commits into from Jul 15, 2020
Merged

Commits on Jul 15, 2020

  1. deps: Upgrade react-native-webview to ~10.0.0.

    We'd like to upgrade to the latest, but versions 10.1.0 and above
    don't have support for RN v0.61; minimum supported is v0.62. (Hence
    the tilde.)
    
    Breaking changes announced:
    
    - v9: iOS: Props updates to `injectedJavaScript` are no longer
      immutable.
    
    We don't use the `injectedJavaScript` prop, so this is fine.
    
    - v10: gradle: The Android Gradle plugin is only required when
      opening the project stand-alone, not when it is included as a
      dependency. [...]
    
    This doesn't sound like a description of a breaking change. As Greg
    said [0], "a thing is no longer required in a case where it
    previously was".
    
    We hesitate to take an x.0.0 version -- what if there are important
    bugfixes in minor/patch versions for issues introduced in that x.0.0
    version? -- but they've made it a custom to post a note at the
    release if they know of major regressions, telling you to use a
    later version [1]. And no such note exists at v10.0.0.
    
    As a concrete example, we were concerned that the "bug fixes"
    identified in the v10.1.0 release might have been aimed at
    regressions introduced in v10.0.0. But following links to the
    mentioned PR and its corresponding issues (the second looks like a
    resumption of the first), it looks like a fix for a bug reported all
    the way back in version 5. So this isn't a reason to use v9.x.x
    instead of v10.0.0.
    
    Greg points out [2]:
    
    """
    In a project that takes major vs. minor releases seriously, where a
    minor release means cherry-picked backported changes, I'd assume
    from this context that the major release introduced the issue.
    
    (Projects like that include RN itself, and the Zulip server.)
    
    But with this project I get the impression that their releases are
    all a linear stream of snapshots from master. Also that they do the
    flavor of "semantic versioning" that's basically "bump the major
    version number frequently and at random". So that makes this less
    clear.
    """
    
    Ah, well.
    
    Also, update the libdef according to changes in
    `src/WebViewTypes.ts` in the `react-native-webview` repo.
    
    [0]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.2060.20upgrade.3A.20react-native-webview/near/893807
    [1]: See, e.g., v10.1.0, at
         https://github.com/react-native-community/react-native-webview/releases/tag/v10.1.0.
    [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E61.3A.20react-native-webview/near/901944
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    41f0b0d View commit details
    Browse the repository at this point in the history
  2. deps: Upgrade react-native-unimodules to ^0.9.1, the latest.

    This is possible, now that the RN v0.60 -> v0.61 upgrade is
    complete! [1]
    
    Change the `@unimodules/core` version, following a valid instruction
    in bc27f2d:
    
    """
    So, declare @unimodules/core in our own "dependencies", with the
    version specified by react-native-unimodules in its active version.
    This matching should be done each time we change versions of
    react-native-unimodules.
    """
    
    Also, remove some now-unnecessary code in `android/build.gradle`, as
    foreshadowed in cb87f90:
    
    """
    Second, specify a new dependency for
    `unimodules-react-native-adapter` in our own `android/build.gradle`,
    in a fix that is necessary because we're locked on version 0.6.0 of
    react-native-unimodules. Filed as
    https://github.com/unimodules/react-native-unimodules/issues/130.
    """
    
    There are some reasonable, automatic changes to the
    version-controlled files that ensure that unimodules are
    automatically linked [2]: `ios/Podfile.lock` and
    `android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java`.
    These look like some modules that are part of
    `react-native-unimodules`' core; these additions are caused by the
    upgrade and don't stem from running something like `yarn add
    expo-apple-authentication` to get a particular Unimodule.
    
    Also, run `yarn yarn-deduplicate && yarn` as prompted by
    `tools/test deps`.
    
    [1]: See discussion of the various obstacles at
         https://github.com/unimodules/react-native-unimodules/issues/97#issuecomment-637714639
         and
         https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Podfile.20dependency.20of.20a.20dependency/near/892373.
    [2]: This is similar to but distinct from `react-native`'s
         "autolinking" feature that we activated in f460460 and
         a9a9ac7. See
         https://github.com/unimodules/react-native-unimodules/issues/75#issuecomment-536517253
         for a comment on that.
    
    Fixes: zulip#4091
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    e116c3f View commit details
    Browse the repository at this point in the history
  3. jest: Add jest-expo preset, to be used in the next commit.

    A bit frustratingly, we need to install react-native-web, because
    it's a peer dep of jest-expo, and react-dom, because that's a peer
    dep of react-native-web.
    
    It's likely that we'll never run any code in either module, since we
    don't care about the web. (Expo seems to be expanding React Native's
    original claim for multi-platform compatibility, namely iOS and
    Android, to include the web. `react-native-web` looks like a thing
    that lets you write React Native...for the web.)
    
    At least they don't complain if we add them as dev dependencies
    instead of regular dependencies.
    
    Also, run `yarn yarn-deduplicate && yarn` as prompted by
    `tools/test deps`.
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    62621ef View commit details
    Browse the repository at this point in the history
  4. jest: Use jest-expo as our preset.

    This calls `react-native`'s Jest setup code before running its own.
    In particular, it seems to be aware of all the Expo modules we might
    want to add using `react-native-unimodules`, and mocks them [1].
    Since we don't need our own mocks for these, remove them.
    
    It seems like we still need to add entries in our
    `transformModulesWhitelist`, ah, well. But it's good to weed out
    boring mocks from our `jestSetup.js`, and leave only those that are
    interesting [2]. Also, it seems like each time we add a module from
    Expo, there's a debugging process that can be confusing [3]; so,
    nice to avoid that.
    
    It looks like the preset does explicitly consider the bare,
    non-"managed" Expo workflow [4], which we use.
    
    If `jest-expo` turns out to be buggy, or the dependency requirements
    get even more tangled or burdensome, we should feel free to abandon
    this effort; it's not terrible to have to add boring mocks.
    
    [1]: https://github.com/expo/expo/blob/b8bd30697/packages/jest-expo/src/preset/expoModules.js
    [2]: Seems like a few remain that aren't related to Expo. Hmm.
    [3]: https://github.com/zulip/zulip-mobile/pull/4034/files#r445956933
    [4]: https://github.com/expo/expo/blob/b8bd30697/packages/jest-expo/src/preset/setup.js#L130
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    e40c020 View commit details
    Browse the repository at this point in the history
  5. jest: Comment about boring mocks, remove one that's unnecessary.

    I'm not sure when exactly it became unnecessary to mock "Linking",
    but it's taken care of now in React Native's Jest setup [1].
    
    [1]: https://github.com/facebook/react-native/blob/v0.61.5/jest/setup.js#L153
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    c1c9575 View commit details
    Browse the repository at this point in the history
  6. Revert "ios: Add boilerplate about APIs we don't, in fact, use."

    This reverts commit c8d0c8d.
    
    As planned [1] [2], now that we've upgraded react-native-unimodules.
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/apple.20purpose.20strings/near/869928
    [2] zulip#4091 (comment)
    chrisbobbe authored and gnprice committed Jul 15, 2020
    Configuration menu
    Copy the full SHA
    e55739e View commit details
    Browse the repository at this point in the history