Add `react-native-unimodules` so we can use Expo packages #4016

chrisbobbe · 2020-04-02T00:20:47Z

Split off from #3987, following Greg's comment:

I think I'd like to split this into two PRs:

CocoaPods, and the simple or docs-only followups to that;

unimodules (as a new PR).

That's because I think the CocoaPods part is close to merging, whereas for unimodules I feel like there's a fair amount going on there where I just need to catch up on some of the reading you've done of its docs and code.

gnprice · 2020-04-03T19:52:34Z

P1 because this is on our path to #3964.

gnprice

Thanks @chrisbobbe ! Comments below, all small.

I've gone and read the unimodules code that gets invoked from our Podfile and Gradle config, too, and I have a better understanding now of what its job is. Looks like it's basically doing the same thing that's more recently arrived for the broader RN ecosystem with "autolinking". Does that sound right from your reading?

yarn.lock

ios/Podfile

gnprice · 2020-04-06T20:14:38Z

ios/ZulipMobile/AppDelegate.m

+  self.moduleRegistryAdapter = [[UMModuleRegistryAdapter alloc] initWithModuleRegistryProvider:[[UMModuleRegistryProvider alloc] init]];
+
  RCTSetLogThreshold(RCTLogLevelError);



Let's keep the "set log threshold" before the newly-added line. Likely the UM code doesn't refer to the RCT log threshold... but if it does (e.g. it calls some RCT-something logging method), then we'll want our choice of log threshold to apply.

(This fix happened in the expo-application commit; moved to this first commit, with git rebase -i / git co -p master $file.)

(This fix happened in the expo-application commit; moved to this first commit, with git rebase -i / git co -p master $file.)

Oh goodness, you're right. Thanks for catching and fixing that!

gnprice · 2020-04-06T20:29:31Z

ios/ZulipMobile/AppDelegate.m

@@ -39,6 +44,14 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
  return YES;
 }

+- (NSArray<id<RCTBridgeModule>> *)extraModulesForBridge:(RCTBridge *)bridge
+{
+  NSArray<id<RCTBridgeModule>> *extraModules = [_moduleRegistryAdapter extraModulesForBridge:bridge];


Here's where I demonstrate that I don't really know how to read Objective-C. 😄 Do you?

It kind of looks like this _moduleRegistryAdapter is referring to the new moduleRegistryAdapter property. But there's that funny leading underscore; and when we initialized what looks like this same property, we spelled it self.moduleRegistryAdapter = ....

Aha, I think this page of Apple's Objective-C docs answers it. I think the short of it is that (a) _moduleRegistryAdapter will work the same way here as self.moduleRegistryAdapter, but (b) in general they may not (e.g. if the class has a more interesting getter for the property -- or if some subclass does), and (c) it's strongly recommended to write self.moduleRegistryAdapter instead, except in certain limited situations that don't apply here.

Let's fix that up, I guess. Probably best as a followup commit -- so as not to crowd this one's commit message, which already has plenty to handle 😉

Do you?

No! 😄

Yep, that reasoning looks good to me; I just read the relevant parts.

gnprice · 2020-04-06T21:07:36Z

ios/Podfile.lock

+  - EXApplication (2.1.0):
+    - UMCore


IIUC how this works, I think this part should happen only in the second commit.

(In this instance, the main value of getting this right is probably that it helps this commit serve as a clean demonstration of how unimodules work! But also I think it avoids having the first commit be broken. So it's also an example of how "every commit keeps things working" helps with "each commit is understandable in itself".)

Makes sense; I definitely want to keep clean commits, both because the first commit is a big step toward enabling a new pattern, and the second commit is the first example of using that pattern (namely, adding a "unimodule"), so lots of people will model their work on it. 🙂

It looks like you've selected lines 4 and 5:

+ - EXApplication (2.1.0): + - UMCore

and these lines were added only in the second commit, if I'm reading my graphical Git client and the GitHub UI correctly. 🙂

Or are you referring to all the additions to the Podfile.lock in the first commit, e.g., EXAppLoaderProvider, EXConstants, EXFileSystem, EXPermissions, and a bunch of things prefixed with UM? If so:

It looks like the use_unimodules script called from the Podfile (it's at node_modules/react-native-unimodules/cocoapods.rb) goes through node_modules and, for any module that has a unimodule.json file in it, includes it with pod "#{pod_name}", path: podspec_directory (the same way other pods are included in the Podfile), which explains how these lines are getting added in the Podfile.lock. So, after running Yarn at this commit, to make sure node_modules is in the correct state, we'll want to be sure we know the reason why every module with a unimodule.json is in node_modules.

We can take exactly the same input, or nearly — just those unimodule.json files, without looking elsewhere in a given module (conveniently, the names in unimodule.json seem to match their counterparts in package.json) — and ask Yarn why they're there:

for f in node_modules/*/unimodule.json; do yarn why $(cat $f | jq -r '.name'); done (I'd appreciate feedback on this little script, if you have any 🙂)

and all of them tell me the same story, namely:

info Reasons this module exists - "react-native-unimodules" depends on it

And 'react-native-unimodules' is, correctly, the only new dependency we're adding to the package.json in this commit.

and these lines were added only in the second commit, if I'm reading my graphical Git client and the GitHub UI correctly. 🙂

Hmm! Indeed they were. Sorry about that -- I misread the branch. Those lines (and the other EXApplication-related lines in this file) were the ones I had in mind.

I think I was actually fooled by the Podfile.lock -diff attribute setting which I asked for on the previous PR <_< >_>... or perhaps by the combination of that and the fact that I have yarn.lock diff in my .git/info/exclude, causing yarn.lock and Podfile.lock to behave differently in the diffs I see.

So in the second commit (which is now the third commit -- the one adding expo-application), I saw it appearing in yarn.lock, meaning it wasn't in node_modules/ before that point... but didn't see any diff in the Podfile.lock -- there would have been a block like this:

diff --git ios/Podfile.lock ios/Podfile.lock index 24f59e6bd..3c581ef7b 100644 Binary files ios/Podfile.lock and ios/Podfile.lock differ

but that's easy to skim past -- and inferred it was already added in the first commit.

I think the immediate lesson is I'll be sure to keep those two files either both at our default of -diff, or both overriden to diff, in my .git/info/exclude so I don't see a spurious mismatch between them.

Sorry about that -- I misread the branch.

No problem! 😄

src/utils/userAgent.js

chrisbobbe · 2020-04-07T01:09:30Z

Looks like it's basically doing the same thing that's more recently arrived for the broader RN ecosystem with "autolinking". Does that sound right from your reading?

Yep! The main work is being done by a Ruby script that calls pod "#{pod_name}", path: podspec_directory for "unimodule" modules (i.e., those that have a unimodule.json in them) in node_modules. The pod ... syntax is the same as we've been using to manually add pods to the Podfile, so it saves that bit of manual work.

At zulip#4016 (comment), Greg pointed out: """ It kind of looks like this `_moduleRegistryAdapter` is referring to the new `moduleRegistryAdapter` property. But there's that funny leading underscore; and when we initialized what looks like this same property, we spelled it `self.moduleRegistryAdapter = ...`. Aha, I think [this page](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html#//apple_ref/doc/uid/TP40011210-CH5-SW1) of Apple's Objective-C docs answers it. I think the short of it is that (a) `_moduleRegistryAdapter` will work the same way here as `self.moduleRegistryAdapter`, but (b) in general they may not (e.g. if the class has a more interesting getter for the property -- or if some subclass does), and (c) it's strongly recommended to write `self.moduleRegistryAdapter` instead, except in certain limited situations that don't apply here. """

chrisbobbe · 2020-04-07T02:12:48Z

Thanks for the review, @gnprice!

OK, I've just made the requested changes, and left a query about clean commits at #4016 (comment). 🙂

gnprice · 2020-04-07T21:45:20Z

Thanks for the revisions! Sorry for the confusion about that Podfile.lock change.

I've made a small tweak mentioned above, and about to merge... oh, and I guess I should actually build and run from the branch first. 😝 Doing that now, then plan to merge assuming all's as expected.

gnprice · 2020-04-07T21:47:35Z

Oh, actually, there's this warning:

$ yarn
yarn install v1.21.1
[1/4] Resolving packages...
[2/4] Fetching packages...
info fsevents@1.2.9: The platform "linux" is incompatible with this module.
info "fsevents@1.2.9" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0".
[4/4] Building fresh packages...
Done in 3.44s.

The unmet peer dependency. Would you find a combination of package versions that avoids that? It's probably fine, this is a simple package, but that general class of issue can cause bugs -- and I like to keep routine flows like yarn install free of warnings.

chrisbobbe · 2020-04-07T21:50:31Z

The unmet peer dependency. Would you find a combination of package versions that avoids that?

Sure! I'll move this to CZO as I expect this to be slightly complicated, given that we're stuck on react-native-unimodules@0.6.0 (no caret)

At zulip#4016 (comment), Greg pointed out: """ It kind of looks like this `_moduleRegistryAdapter` is referring to the new `moduleRegistryAdapter` property. But there's that funny leading underscore; and when we initialized what looks like this same property, we spelled it `self.moduleRegistryAdapter = ...`. Aha, I think [this page](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html#//apple_ref/doc/uid/TP40011210-CH5-SW1) of Apple's Objective-C docs answers it. I think the short of it is that (a) `_moduleRegistryAdapter` will work the same way here as `self.moduleRegistryAdapter`, but (b) in general they may not (e.g. if the class has a more interesting getter for the property -- or if some subclass does), and (c) it's strongly recommended to write `self.moduleRegistryAdapter` instead, except in certain limited situations that don't apply here. """

This lets us use individual Expo packages. In particular, we plan to use expo-apple-authentication soon, for in place of some existing dependencies, such as (non-exhaustively) react-native-safe-area and react-native-orientation. So, add it, without yet using it, following instructions at https://github.com/unimodules/react-native-unimodules/blob/30da302a8/README.md. It looks like Unimodules has also gone by the name Universal Modules, but this may be an old name that has been superseded: unimodules/unimodules.org@23c53dd...bdc80d9 Note that we omit the caret in 0.6.0, to disallow silent minor version upgrades. The maintainers are not adhering to semantic versioning, and they expect that you're using the latest version of React Native if you're using the latest version of unimodules, including minor versions; see https://github.com/unimodules/react-native-unimodules/issues/100#issuecomment-566436685. In that same comment, a maintainer says that 0.7.0-rc.4 is the minimum version with AndroidX compatibility. If this is true, then we may have a slight problem, because 0.7.0-rc.4, just like 0.6.0, does not work with RN 0.59.10. That would mean we had to do the AndroidX upgrade (PR zulip#3852) at the same time as the RN v0.60 upgrade (issue zulip#3548). Greg thinks it's likely that the maintainer is mistaken that there's a minimum version of unimodules required to use AndroidX; the AndroidX engineers designed a smooth migration path such that this shouldn't happen (see Greg's comment at zulip#3987 (comment)). A later commit in this series describes another concern with Expo's handling of version constraints, with expo-application (and others), with a reference to expo/expo#7728, which I just filed. A couple of fixes had to be made to get it to work on Android: First, pass an additional argument to a constructor in MainApplication.java, in a fix given by a maintainer at https://github.com/unimodules/react-native-unimodules/issues/128#issuecomment-606027692. 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. Also (not Android-specific), ran `yarn yarn-deduplicate` to deduplicate `ua-parser-js`, following a prompt from `tools/test deps`. Fixes: zulip#3987

At zulip#4016 (comment), Greg pointed out: """ It kind of looks like this `_moduleRegistryAdapter` is referring to the new `moduleRegistryAdapter` property. But there's that funny leading underscore; and when we initialized what looks like this same property, we spelled it `self.moduleRegistryAdapter = ...`. Aha, I think [this page](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/EncapsulatingData/EncapsulatingData.html#//apple_ref/doc/uid/TP40011210-CH5-SW1) of Apple's Objective-C docs answers it. I think the short of it is that (a) `_moduleRegistryAdapter` will work the same way here as `self.moduleRegistryAdapter`, but (b) in general they may not (e.g. if the class has a more interesting getter for the property -- or if some subclass does), and (c) it's strongly recommended to write `self.moduleRegistryAdapter` instead, except in certain limited situations that don't apply here. """

Primarily to prove the correctness of our `react-native-unimodules` installation, install expo-application and use it instead of `react-native-device-info` in those places where it reads the application's version. NOTE: After discussing it at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/unimodules.20unmet.20peer.20dependency/near/849254, we decided to ignore the following warning: warning " > expo-application@2.1.0" has unmet peer dependency "@unimodules/core@~1.0.0". I filed expo/expo#7728 for it, but basically, the latest version of expo-application (2.1.0) declares that it depends on a version of @unimodules/core (1.0.0) that was already several months out-of-date at the time expo-application was first created, and it seems unlikely that this was well-considered. In our testing so far, everything seems to work correctly, and the vast majority of unimodules don't specify a version constraint at all. But this adds to the need to be wary about Expo's handling of version constraints in general; see the notes on the commit, earlier in this series, that introduces unimodules. To choose between `nativeApplicationVersion` and `nativeBuildVersion`, the deciding factor was which one of these more closely mirrors the behavior of `getVersion` from `react-native-device-info`. (`getVersion` isn't documented well enough to give the answer outright.) `react-native-device-info` is missing documentation on where they get the version number, so we dug into the code and found it: - iOS: `CFBundleShortVersionString` from `info.plist` - Android: `PackageInfo#versionName` (doc at https://developer.android.com/reference/android/content/pm/PackageInfo#versionName) From inspecting `expo-application` code, it turns out that these same exact sources are used for their `nativeApplicationVersion`. The `expo-application` doc for Android actually refers to an `app.json`, which we don't have, and it doesn't mention `versionName`. Maintaining an `app.json` is a normal part of development when you're working entirely in the Expo ecosystem (which we're not), and it's documented (https://docs.expo.io/versions/latest/workflow/configuration/#version) that the "version" key there does indeed correspond to the `versionName` we found by looking in the code. So, use `nativeApplicationVersion`. We don't expect `nativeApplicationVersion` to ever be null, but the type annotation indicates it might be, so handle that case with a "?.?.?" string.

An odd-looking conditional was introduced in 9d9720c; from experimentation, it looks like this was to prevent some failures while running our Jest test suite. Some items in NativeModules from react-native aren't present when Jest is running, so we get errors if, on startup, we try to run functions that depend on them being present. The solution, as we've done for several others, including `expo-application`, in the parent, is to mock the JS modules that provide these functions.

In 33f4b41, we intended to be on 1.9.1, and I did all the work of that PR using 1.9.1. Greg was using 1.8.4, and when he was fixing up my branch, he accidentally committed the Podfile.lock with 1.8.4. But we have no reason not to use this latest version, so, do.

gnprice · 2020-04-08T20:30:23Z

And merged. Thanks!

chrisbobbe requested a review from gnprice April 2, 2020 00:20

This was referenced Apr 2, 2020

Use CocoaPods #3987

Merged

Upgrade to RN v0.60 #3548

Closed

chrisbobbe force-pushed the pr-add-unimodules branch 2 times, most recently from e3e58db to 48ef3e4 Compare April 3, 2020 00:33

gnprice added the P1 high-priority label Apr 3, 2020

chrisbobbe force-pushed the pr-add-unimodules branch from 48ef3e4 to e0eb04c Compare April 3, 2020 21:30

gnprice reviewed Apr 6, 2020

View reviewed changes

chrisbobbe force-pushed the pr-add-unimodules branch from e0eb04c to ed7e06a Compare April 7, 2020 02:10

gnprice force-pushed the pr-add-unimodules branch from ed7e06a to 4fccff5 Compare April 7, 2020 21:50

chrisbobbe force-pushed the pr-add-unimodules branch from 4fccff5 to edf09bd Compare April 8, 2020 00:11

chrisbobbe mentioned this pull request Apr 8, 2020

CocoaPods near-term followup #4028

Merged

Chris Bobbe added 5 commits April 8, 2020 13:29

gnprice force-pushed the pr-add-unimodules branch from edf09bd to d91fe17 Compare April 8, 2020 20:30

gnprice merged commit d91fe17 into zulip:master Apr 8, 2020

chrisbobbe mentioned this pull request Apr 8, 2020

Support "Sign In with Apple" auth [iOS] #3964

Closed

chrisbobbe mentioned this pull request May 19, 2020

Replace unmaintained dep react-native-orientation. #4118

Closed

chrisbobbe deleted the pr-add-unimodules branch November 6, 2020 03:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `react-native-unimodules` so we can use Expo packages #4016

Add `react-native-unimodules` so we can use Expo packages #4016

chrisbobbe commented Apr 2, 2020

gnprice commented Apr 3, 2020

gnprice left a comment

gnprice Apr 6, 2020

gnprice Apr 7, 2020

chrisbobbe Apr 7, 2020

gnprice Apr 6, 2020

chrisbobbe Apr 7, 2020

gnprice Apr 6, 2020

chrisbobbe Apr 7, 2020 •

edited

Loading

gnprice Apr 7, 2020 •

edited

Loading

chrisbobbe Apr 7, 2020 •

edited

Loading

chrisbobbe commented Apr 7, 2020

chrisbobbe commented Apr 7, 2020

gnprice commented Apr 7, 2020

gnprice commented Apr 7, 2020

chrisbobbe commented Apr 7, 2020

gnprice commented Apr 8, 2020

		self.moduleRegistryAdapter = [[UMModuleRegistryAdapter alloc] initWithModuleRegistryProvider:[[UMModuleRegistryProvider alloc] init]];

		RCTSetLogThreshold(RCTLogLevelError);

Add react-native-unimodules so we can use Expo packages #4016

Add react-native-unimodules so we can use Expo packages #4016

Conversation

chrisbobbe commented Apr 2, 2020

gnprice commented Apr 3, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice Apr 6, 2020

Choose a reason for hiding this comment

gnprice Apr 7, 2020

Choose a reason for hiding this comment

chrisbobbe Apr 7, 2020

Choose a reason for hiding this comment

gnprice Apr 6, 2020

Choose a reason for hiding this comment

chrisbobbe Apr 7, 2020

Choose a reason for hiding this comment

gnprice Apr 6, 2020

Choose a reason for hiding this comment

chrisbobbe Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

gnprice Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Apr 7, 2020

chrisbobbe commented Apr 7, 2020

gnprice commented Apr 7, 2020

gnprice commented Apr 7, 2020

chrisbobbe commented Apr 7, 2020

gnprice commented Apr 8, 2020

Add `react-native-unimodules` so we can use Expo packages #4016

Add `react-native-unimodules` so we can use Expo packages #4016

chrisbobbe Apr 7, 2020 •

edited

Loading

gnprice Apr 7, 2020 •

edited

Loading

chrisbobbe Apr 7, 2020 •

edited

Loading