Page MenuHomePhabricator

[native] [32/40] RN 0.70: Working AndroidLifecycle Expo module
ClosedPublic

Authored by ashoat on Dec 19 2022, 7:04 AM.
Tags
None
Referenced Files
F3230923: D5933.id19638.diff
Tue, Nov 12, 9:38 AM
Unknown Object (File)
Sun, Nov 10, 5:54 PM
Unknown Object (File)
Thu, Nov 7, 5:53 AM
Unknown Object (File)
Wed, Nov 6, 6:26 AM
Unknown Object (File)
Mon, Nov 4, 4:09 PM
Unknown Object (File)
Mon, Nov 4, 4:09 PM
Unknown Object (File)
Mon, Nov 4, 4:09 PM
Unknown Object (File)
Mon, Nov 4, 4:09 PM
Subscribers

Details

Summary

Baby's first Kotlin! Context about this new Expo module in D5932.

Depends on D5932

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.ACTIVE: ${AndroidLifecycleModule.ACTIVE ?? 'null or undefined'}`);
  console.log(`androidLifecycle.initialStatus: ${AndroidLifecycleModule.initialStatus ?? 'null or undefined'}`);
  AndroidLifecycleModule.addAndroidLifecycleListener(status => { console.log(`onChange ${status}`) });
}

It resulted in the following output after I started, then backgrounded, and then foregrounded the app:

LOG  androidLifecycle.ACTIVE: active
LOG  androidLifecycle.initialStatus: active
LOG  onChange background
LOG  onChange active

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 19 2022, 7:05 AM
Harbormaster failed remote builds in B14317: Diff 19560!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 19 2022, 9:11 AM
Harbormaster failed remote builds in B14389: Diff 19638!

Please ignore CI until the end of the stack

bartek requested changes to this revision.Dec 19 2022, 12:05 PM

Congratz on the first expo-module! I left a few nits about Kotlin / expo-module conventions. I'm probably a maniac when it comes to Kotlin code style, so treat my comments as optional and just let me know if you're gonna skip them ;)

native/expo-modules/android-lifecycle/android/build.gradle
95 ↗(On Diff #19673)

Wasn't this + recently problematic when RN was published to Maven and this was resolving to wrong version?

native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt
12–15 ↗(On Diff #19673)

IMO string names are typo-prone, an enum would be more suitable here, see suggestion. This is unfortunately more verbose approach, so up to you

The enum can be top-level, it doesn't have to be nested inside a class

Example usages:

val state = LifecycleState.ACTIVE; // 'state' is of type LifecycleState
val stateName = state.jsName; // "active"
val directName = LifecycleState.ACTIVE.jsName; // same as above

// this generates a map that you initially wrote
val lifecycleStates = LifecycleState.values().associate {
  it.name to it.jsName
}

// the above can be embedded as an enum static (companion) value
private enum class LifecycleState(val jsName: String) {
    ACTIVE("active"),
    BACKGROUND("background");
    
   companion object {
      val constantsMap
         get() = LifecycleState.values().associate { it.name to it.jsName }
    }
}
// then you can do
val constants = LifecycleState.constantsMap // this is of type Map<String, String>
16–17 ↗(On Diff #19673)

There is a convention to use top-level constants: private const val above the class definition. Example here. It's good to have all string constant names in one place.

The const val defines a compile-time constant.

40–41 ↗(On Diff #19673)

Nitpicks about expo-module convention:

  • The definition() is usually the first function inside the module class. This way it is clear that this class defines an expo-module
  • Module name can be extracted as a top-level constant
44–49 ↗(On Diff #19673)

If the suggestion about enum is applied, then it can be done this way

54–58 ↗(On Diff #19673)

Nit:
I don't know if you consider this as more clean/readable, but in Kotlin we prefer to omit parens ( ) when a closure is the only/last param of a function:

OnStartObserving {
  // ...
}

The same for Constants and OnStopObserving

This revision now requires changes to proceed.Dec 19 2022, 12:05 PM

Will apply the Kotlin comments

native/expo-modules/android-lifecycle/android/build.gradle
95 ↗(On Diff #19673)

@bartek is referring to this: https://github.com/facebook/react-native/issues/35210

I agree this line is an anti-pattern, but I'm not sure what I should replace it with. I'm basing this on how Expo modules do it, eg. expo-av needs UiThreadUtil and it does the same thing.

One thought is that I could use this approach to get REACT_NATIVE_VERSION, and use that instead of the +. (Strange that I'm linking the same file in expo-av again... they are already getting REACT_NATIVE_VERSION, so I wonder why it's not used in the dependencies block.)

Anyways, I'm not sure how much more I should investigate it given it appears like a standard approach. Let me know if you think it needs more investigation, or if you have a suggestion on what line to replace this with.

bartek added inline comments.
native/expo-modules/android-lifecycle/android/build.gradle
95 ↗(On Diff #19673)

I was just curious why it actually worked, but my suspicion is that this is how Gradle works - if the exact version is provided once, then all + resolve to that version instead of downloading the latest, for instance.

Anyway, if the expo libraries still use the same approach, so I'd leave it as is.

This revision is now accepted and ready to land.Dec 19 2022, 11:28 PM
tomek added inline comments.
native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt
35 ↗(On Diff #19769)

What does this syntax mean? I guess it is Constants annotations applied to the value, but can't be sure. But if that's the case, can we format it so that there's a space between it and return keyword?

native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt
35 ↗(On Diff #19769)

@Constants is actually applied to return. It's saying "return the closure passed to Constants" as opposed to eg. "return the closure passed to ModuleDefinition"

native/expo-modules/android-lifecycle/android/src/main/java/app/comm/android/lifecycle/AndroidLifecycleModule.kt
35 ↗(On Diff #19769)

Yes, it is called labeled returns, useful when having nested closures.

Another way would be to omit the return@Constants completely, as, similar to Rust, the last expression is returned from the closure. But in my opinion, that is a less obvious way, at least in this case (it is good in one-liner closures).