Details
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
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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. |
| 40–41 ↗ | (On Diff #19673) | Nitpicks about expo-module convention:
|
| 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: OnStartObserving { // ... } The same for Constants and OnStopObserving |
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. |
| 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. |
| 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). |