Page MenuHomePhabricator
Differential D5932

[native] [31/40] RN 0.70: First commit for new AndroidLifecycle Expo module
ClosedPublic
Actions

Authored by ashoat on Dec 19 2022, 7:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 4:11 AM
Unknown Object (File)
Mon, Mar 31, 10:24 AM
Unknown Object (File)
Wed, Mar 26, 7:25 AM
Unknown Object (File)
Sat, Mar 15, 7:46 AM
Unknown Object (File)
Mon, Mar 10, 12:32 AM
Unknown Object (File)
Mon, Mar 10, 12:32 AM
Unknown Object (File)
Mon, Mar 10, 12:32 AM
Unknown Object (File)
Mon, Mar 10, 12:32 AM
View All Files
Subscribers
atul

Details

Reviewers
bartek
tomek
Commits
rCOMMef97f1fc8578: [native] [31/40] RN 0.70: First commit for new AndroidLifecycle Expo module
Summary

We have an existing custom AndroidLifecycle React Native module, but it started having issues after the update: ENG-2484. I decided to migrate the module to an Expo module, which are newly supported as of Expo 47.

This diff contains basically an end-to-end "Hello World", mostly pulled from running npx create-expo-module expo-settings and filtering out just the pieces I wanted to keep.

Depends on D5925

Test Plan

I patched the following into native/root.react.js:

import * as AndroidLifecycleModule from './lifecycle/lifecycle-module';

if (Platform.OS === 'android') {
  console.log(`androidLifecycle.PI: ${AndroidLifecycleModule.PI ?? 'null or undefined'}`);
  console.log(`androidLifecycle.hello(): ${AndroidLifecycleModule.hello() ?? 'null or undefined'}`);
  (async () => {
    AndroidLifecycleModule.addChangeListener(({ value }) => { console.log(`onChange ${value}`) });
    await AndroidLifecycleModule.setValueAsync('testing!!');
    console.log('done testing');
  })();
}

It resulted in the following output:

LOG  androidLifecycle.PI: 3.141592653589793
LOG  androidLifecycle.hello(): Hello world! 👋
LOG  onChange testing!!
LOG  done testing

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Dec 19 2022, 7:00 AM
Herald added a subscriber: atul. · View Herald TranscriptDec 19 2022, 7:00 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 19 2022, 7:01 AM
Harbormaster failed remote builds in B14316: Diff 19558!
ashoat added a child revision: D5933: [native] [32/40] RN 0.70: Working AndroidLifecycle Expo module.Dec 19 2022, 7:04 AM
ashoat mentioned this in D5934: [native] [33/40] RN 0.70: Replace old AndroidLifecycleModule with new Expo one.
ashoat updated this revision to Diff 19604.Dec 19 2022, 8:22 AM

Rebase

ashoat updated this revision to Diff 19637.Dec 19 2022, 8:38 AM

Rebase

Harbormaster failed remote builds in B14355: Diff 19604!Dec 19 2022, 8:41 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 19 2022, 9:10 AM
Harbormaster failed remote builds in B14388: Diff 19637!
ashoat requested review of this revision.Dec 19 2022, 9:13 AM

Please ignore CI until the end of the stack

native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt
1–38 ↗(On Diff #19637)

I shorten the line lengths to 80 chars in the next diff. This is just boilerplate

ashoat added inline comments.Dec 19 2022, 9:59 AM
package.json
12 ↗(On Diff #19637)

Oops, this was a rebase mistake

13 ↗(On Diff #19637)

I need to copy this over in keyserver/Dockerfile. That's what's causing the CI failure there

ashoat updated this revision to Diff 19661.Dec 19 2022, 10:01 AM

Copy new package.json in keyserver Dockerfile, remove reference to deprecated opaque-ke-node package

Harbormaster failed remote builds in B14410: Diff 19661!Dec 19 2022, 10:03 AM
ashoat updated this revision to Diff 19672.Dec 19 2022, 10:23 AM

Modify .dockerignore to make sure Docker can access new package.json

bartek accepted this revision.Dec 19 2022, 10:49 AM
This revision is now accepted and ready to land.Dec 19 2022, 10:49 AM
Harbormaster failed remote builds in B14420: Diff 19672!Dec 19 2022, 11:06 AM
ashoat updated this revision to Diff 19710.Dec 19 2022, 12:30 PM

Rebase

Harbormaster completed remote builds in B14455: Diff 19710.Dec 19 2022, 12:53 PM
ashoat updated this revision to Diff 19768.Dec 20 2022, 6:23 AM

Rebase

Harbormaster completed remote builds in B14511: Diff 19768.Dec 20 2022, 6:45 AM
tomek accepted this revision.Dec 20 2022, 7:15 AM
tomek added inline comments.
package.json
44–46 ↗(On Diff #19768)

What is changed in these lines? (Phabricator isn't too helpful here)

ashoat added inline comments.Dec 20 2022, 7:21 AM
package.json
44–46 ↗(On Diff #19768)

I think I accidentally copy-pasted tab characters in D5901. I can move this change there before landing.

ashoat@ashoatmbp2021 [~/src/comm]$ git diff HEAD^ package.json
diff --git a/package.json b/package.json
index 93e34aba0..f87fd501a 100644
--- a/package.json
+++ b/package.json
@@ -8,7 +8,8 @@
     "keyserver",
     "landing",
     "desktop",
-    "keyserver/addons/opaque-ke-napi"
+    "keyserver/addons/opaque-ke-napi",
+    "native/expo-modules/android-lifecycle"
   ],
   "scripts": {
     "clean": "yarn workspace lib clean && yarn workspace web clean && yarn workspace native clean && yarn workspace keyserver clean && yarn workspace landing clean && yarn workspace desktop clean && yarn workspace opaque-ke-napi clean && rm -rf node_modules/",
@@ -41,6 +42,6 @@
     "prettier": "^2.1.2"
   },
   "resolutions": {
-               "react-native-flipper": "https://registry.yarnpkg.com/@favware/skip-dependency/-/skip-dependency-1.0.2.tgz"
-       }
+    "react-native-flipper": "https://registry.yarnpkg.com/@favware/skip-dependency/-/skip-dependency-1.0.2.tgz"
+  }
 }
ashoat updated this revision to Diff 19830.Dec 20 2022, 11:03 AM

Rebase

Harbormaster failed remote builds in B14570: Diff 19830!Dec 20 2022, 11:20 AM
ashoat added inline comments.Dec 20 2022, 11:20 AM
package.json
44–46 ↗(On Diff #19768)

Tab-to-spaces change has been moved to D5901 where it belongs

Harbormaster completed remote builds in B14570: Diff 19830.Dec 20 2022, 11:55 AM
Closed by commit rCOMMef97f1fc8578: [native] [31/40] RN 0.70: First commit for new AndroidLifecycle Expo module (authored by ashoat). · Explain WhyDec 20 2022, 12:01 PM
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMMef97f1fc8578: [native] [31/40] RN 0.70: First commit for new AndroidLifecycle Expo module.
ashoat mentioned this in rCOMM78223f2dd741: [native] [33/40] RN 0.70: Replace old AndroidLifecycleModule with new Expo one.
bartek mentioned this in D7002: [native] Create AESCrypto Expo module.Mar 8 2023, 9:51 AM
bartek mentioned this in rCOMM8735eb5f5924: [native] Create AESCrypto Expo module.Mar 20 2023, 7:15 AM

Revision Contents
Changeset List

PathSize
keyserver/
2 lines
native/
expo-modules/
android-lifecycle/
android/
91 lines
src/
main/
2 lines
java/
app/
comm/
android/
lifecycle/
38 lines
6 lines
16 lines
lifecycle/
36 lines
1 line
7 lines
902 lines

Diff 19661

View Options

keyserver/Dockerfile

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/expo-modules/android-lifecycle/android/build.gradle

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/expo-modules/android-lifecycle/android/src/main/AndroidManifest.xml

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/expo-modules/android-lifecycle/expo-module.config.json

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/expo-modules/android-lifecycle/package.json

Loading...
View Options

native/lifecycle/lifecycle-module.js

Loading...
View Options

native/package.json

Loading...
View Options

package.json

Loading...
View Options

yarn.lock

Loading...