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

Replace unmaintained dep react-native-orientation. #4118

Closed
chrisbobbe opened this issue May 19, 2020 · 8 comments · Fixed by #4147
Closed

Replace unmaintained dep react-native-orientation. #4118

chrisbobbe opened this issue May 19, 2020 · 8 comments · Fixed by #4147
Labels
dependencies Pull requests that update a dependency file upstream: other Issues related to an issue in another dependency

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented May 19, 2020

When we upgrade to RN v0.60.0 (that's #3548), we'll get this warning:

warn The following packages use deprecated "rnpm" config that will stop working from next release:
  - react-native-orientation: https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
Please notify their maintainers about it. You can find more details at https://github.com/react-native-community/cli/blob/master/docs/configuration.md#migration-guide.

rn-fetch-blob is easy to fix; we just have to upgrade to v0.11; I'm planning to do that in my PR for #3548.

react-native-orientation is unmaintained, though. The fix for this warning is very simple, but the PR for it has gathered dust, and the repo isn't getting updates.

This will take some assembly in native code, which should all be spelled out in the replacement library's installation instructions.

I recommend trying Expo's ScreenOrientation; Expo libraries are generally frequently maintained, and we can newly use them since #4016 landed. If we're going with that, we'll follow the instructions for a "bare React Native app" (ours isn't "managed" by Expo), and use yarn add expo-screen-orientation instead of npm install. Since Expo uses TypeScript instead of Flow, we'll have to put together a libdef, unless FlowTyped has one (I don't think it does). We have a new doc for that, and it would be nice for that to be updated with anything new we learn along the way.

@chrisbobbe chrisbobbe added upstream: other Issues related to an issue in another dependency dependencies Pull requests that update a dependency file labels May 19, 2020
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 3, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

A lot of work has already been done toward the upgrade:

- most of the fixes necessary to adapt to Flow 0.98, without
  upgrading Flow yet (zulip#3827). We do the upgrade in this commit, with
  warnings temporarily suppressed, for smaller commits.

- updating to AndroidX (zulip#3852)

- migrating to using CocoaPods at all (zulip#3987)

- We ignore or have already handled several changes, e.g., to the
  Xcode and Gradle configs (see comment at zulip#3548).

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 4, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

We've already done a lot of the work toward the upgrade, and we've
ignored some changes suggested by the upgrade helper. See comments
on zulip#3548.

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 5, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

We've already done a lot of the work toward the upgrade, and we've
ignored some changes suggested by the upgrade helper. See comments
on zulip#3548.

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548

Fixes: zulip#4093
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 5, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

We've already done a lot of the work toward the upgrade, and we've
ignored some changes suggested by the upgrade helper. See comments
on zulip#3548.

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548

Fixes: zulip#4093
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 8, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

We've already done a lot of the work toward the upgrade, and we've
ignored some changes suggested by the upgrade helper. See comments
on zulip#3548.

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548
Fixes: zulip#4093
@chrisbobbe
Copy link
Contributor Author

Huh, it would seem that we may actually want DeviceMotion, not ScreenOrientation...

From the ScreenOrientation doc:

Screen Orientation is defined as the orientation in which graphics are painted on the device. For example, the figure below has a device in a vertical and horizontal physical orientation, but a portrait screen orientation. For physical device orientation, see the orientation section of Device Motion.

gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 8, 2020
Using the RN Upgrade Helper, a web app showing the diff from the
release/0.59.10 and the release/0.60.6 branches of the
`react-native-community/rn-diff-purge` repo, at
https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6.

In this commit:

- Upgrade `react-native`, `react`, and `flow-bin` following the
  templates

- Upgrade `react-native-webview` to satisfy peer dependencies

- Adapt our Podfile to RN v0.60's new layout of pods, following the
  templates

- Upgrade `react-native-sound` and `rn-fetch-blob` to versions with
  podspecs compatible with the new pod layout in RN v0.60

We've already done a lot of the work toward the upgrade, and we've
ignored some changes suggested by the upgrade helper. See comments
on zulip#3548.

Note: The following warning appears when running Metro and will be
fixed with follow-up work (e.g., zulip#4118):

```
warn The following packages use deprecated "rnpm" config that will
stop working from next release:
  - react-native-orientation:
    https://github.com/yamill/react-native-orientation#readme
  - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob
```

----- Platform-agnostic --------------------------------------------

We upgrade `react-native-webview` in this commit because versions
before 7.x aren't compatible with RN v0.60, and versions 7.x aren't
compatible with RN v0.59 (judging by peer dependency warnings). We
take the latest 7.x version, 7.6.0.

Nicely, this gets us the changes from one of our PRs, released in
7.0.3; see 1982f3f and its reversion in the commit that followed,
bbfac73.

There's just one declared breaking change [1] in 7.x.x:

- UIWebView removed (7.0.1).

This prompted the `scalesPageToFit` prop to be removed, but we don't
use it. The `useWebKit` prop was also removed because it doesn't
make sense for it to be anything but `true` now. So, remove our use
of it.

Also run `yarn yarn-deduplicate && yarn`, as prompted by
`tools/test deps`.

[1]: https://github.com/react-native-community/react-native-webview#versioning

----- Android ------------------------------------------------------

There are no updates on the Android side that must happen atomically
with the RN upgrade.

----- iOS ----------------------------------------------------------

facebook/react-native@2321b3fd7 appears to be the only cause of iOS
changes that must happen atomically with the RN v0.60 upgrade.

In that commit, all the "subspecs" that were nested under the
"React" pod are un-nested so they each become their own pod, with
many of these living in their own directory under
node_modules/react-native/Libraries [1].

In our own code, this means changing our Podfile to refer to all
these new pods, instead of the "React" pod's subspecs. So, do.

Our Podfile has a list of "Pods we need that depend on React
Native". We must also be sure all the dependencies in this list
adapt to facebook/react-native@2321b3fd7.

The syntax for pointing explicitly to a subspec is a slash, as in
"React/Core" [2]. Knowing this, we check that list for pods that
explicitly depended on those subspecs, with "React/[...]":

```
grep -Rl dependency.*React/ --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

There are two, and they both have new versions that adapt to the new
accepted shape:

- `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React"
- `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core"

So, take these new versions, and job done.

[1]: They do still live in node_modules. It's possible for pods to
     be hosted online and downloaded on `pod install`, npm-style.
     For an example, see the 'Toast' dependency in
     node_modules/react-native-simple-toast/react-native-simple-toast.podspec.
     But that's not what's happening here, yet.
     facebook/react-native@2321b3fd7 hints that this will be the way
     of the future for React Native, "to make the experience nicer
     for library consumers".

[2]: https://guides.cocoapods.org/syntax/podspec.html#subspec

Fixes: zulip#3548
Fixes: zulip#4093
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 8, 2020

Huh, it would seem that we may actually want DeviceMotion, not ScreenOrientation...

Ah, no, actually (at least on iOS; haven't tested on Android yet) — when I followed some alternate suggestions for the AppDelegate.m, defaulting to a lock of ALL orientations allowed, it does indeed respond to physical device rotation

And, it works on Android too!

@gnprice
Copy link
Member

gnprice commented Jun 9, 2020

Cool. I suspect what that means is: "orientation" means how the display is oriented on the device -- which is often controlled by how the device is oriented physically, but may not be (e.g. with "orientation lock" mode.)

So if you really are interested in how the device is oriented physically relative to gravity, you want the other thing -- but we're interested in how the display is oriented on the device.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 9, 2020
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 9, 2020

but may not be (e.g. with "orientation lock" mode.)

Yeah. What isn't clear in their setup instructions for what to put in AppDelegate.m is that the "default orientation" really seems to mean an orientation you'd like to lock to, and have difficulty changing to a different lock later. So I locked to allowing "all" orientations.

The line in the instructions that threw me off, I think, is this, from here:

or if you want to change the default screen orientation, with:

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 9, 2020

Hmm, that's not quite it. That tweak seems to affect the supported device orientations, which makes sense—not "orientations you'd like to lock to and have difficulty changing later". See the Apple doc. (Incidentally, it would be quite easy to support all orientations except upside-down, if we think it's a bit silly to allow the upside-down orientation.)

@gnprice
Copy link
Member

gnprice commented Jun 9, 2020

Yeah. What isn't clear in their setup instructions for what to put in AppDelegate.m is that the "default orientation" really seems to mean an orientation you'd like to lock to, and have difficulty changing to a different lock later. So I locked to allowing "all" orientations.

The line in the instructions that threw me off, I think, is this

That line does sound pretty explicit!

Have you been trying this empirically? (Seems like the cases of interest are tatus quo, simple/default version of their setup, and version with initWithDefaultScreenOrientationMask:UIInterfaceOrientationMaskAll.) What did you find the behavior turns out to be?

There's this other bit of docs on this Expo module -- though on a different part of its API:

OrientationLock.DEFAULT -- The default orientation. On iOS, this will allow all orientations except Orientation.PORTRAIT_DOWN. On Android, this lets the system decide the best orientation.

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 9, 2020

Have you been trying this empirically?

OK, just took another look at this. Turns out there's some unexplained variation (addressed toward the bottom)!

  • Status quo (no change to AppDelegate.m): The screen orientation responds to the physical device's rotation, but our listener that dispatches APP_ORIENTATION doesn't get triggered.

  • With this change:

- UIViewController *rootViewController = [UIViewController new];
+ UIViewController *rootViewController = [[EXScreenOrientationViewController alloc] init];

The screen orientation does not respond to the physical device's rotation, and our listener isn't triggered.

  • With this change:
- UIViewController *rootViewController = [UIViewController new];
+ UIViewController *rootViewController =  [[EXScreenOrientationViewController alloc] initWithDefaultScreenOrientationMask:UIInterfaceOrientationMaskAll];

The screen orientation responds to the physical device's rotation, and our listener gets triggered. 👍

So, that seems consistent with the Apple doc that says we're specifying the "supported" device orientations.

The "unexplained variation" is in the behavior of ScreenOrientation.lockAsync(orientationLock) for valid values of orientationLock (0-9).

I've set up this interval:

    setInterval(() => {
      ScreenOrientation.lockAsync(Math.floor(Math.random() * 10));
    }, 1000);

and I'm not seeing consistent behavior within any particular one of those three cases. Sometimes it does rotate randomly every second, sometimes it doesn't. I do restart the Xcode build each time (since it's a change to the native code). I've tried clearing the build folder and building again. Sometimes it changes from the unexpected behavior to the expected behavior when I do something like disable remote JS debugging. Sometimes not. Weird.

Anyway, we're not interested in using lockAsync, and we see the behavior we want, in that third case. So I don't think I necessarily need to pin this down further; what do you think, @gnprice?

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 9, 2020
@gnprice
Copy link
Member

gnprice commented Jun 9, 2020

Anyway, we're not interested in using lockAsync, and we see the behavior we want, in that third case. So I don't think I necessarily need to pin this down further; what do you think, @gnprice?

Yep, sgtm! Thanks for pinning down the behavior of the part we need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file upstream: other Issues related to an issue in another dependency
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants