-
Notifications
You must be signed in to change notification settings - Fork 13
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #317 from bitmovin/PRN-74/refactor--improve-null-s…
…afety ## Problem (PRN-74) The codebase had many issues: - invalid null parameters were ignored instead of reporting an error - invalid state (module not loaded) were ignored instead of reporting an error - many functions were taking unnecessary null parameters, returning null unnecessarily - errors were not reported - some promise were not resolve (deadlock if awaited) - exception do not reject promisses ## Changes Simplify the code and improve error visibility by: - Errors are bubbled up to the caller with `@ReactNative`'s `promise.reject` instead of ignoring them or logging them. - Refactor json serialization to use null operator `?.` rather than operating on null values. - Make `toJson` extension function to easily chain `?.` operations - Make `Json.toXXX` extension function to easily chain `?.` operations - Add `XXXModule` variant that throws an exception if the module are not loaded, which should never happen - Wrap all `@ReactNative` methods in a catch to report any exception instead of crashing the app - Introduce a base Bitmovin module for helper functions - Wrap promise in a strongly typed interface - All `@ReactCommand` promisses are guarantied to be resolved or rejected - Exception in the PlayerViewManager are logged instead of crashing the app ## Checklist - [x] 🗒 `CHANGELOG` entry if applicable
- Loading branch information
Showing
20 changed files
with
1,362 additions
and
1,871 deletions.
There are no files selected for viewing
106 changes: 106 additions & 0 deletions
106
android/src/main/java/com/bitmovin/player/reactnative/BitmovinBaseModule.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| package com.bitmovin.player.reactnative | ||
|
|
||
| import android.util.Log | ||
| import com.bitmovin.player.api.Player | ||
| import com.bitmovin.player.api.source.Source | ||
| import com.bitmovin.player.reactnative.extensions.drmModule | ||
| import com.bitmovin.player.reactnative.extensions.offlineModule | ||
| import com.bitmovin.player.reactnative.extensions.playerModule | ||
| import com.bitmovin.player.reactnative.extensions.sourceModule | ||
| import com.bitmovin.player.reactnative.extensions.uiManagerModule | ||
| import com.facebook.react.bridge.* | ||
| import com.facebook.react.uimanager.UIManagerModule | ||
|
|
||
| private const val MODULE_NAME = "BitmovinBaseModule" | ||
|
|
||
| /** | ||
| * Base for Bitmovin React modules. | ||
| * | ||
| * Provides many helper methods that are promise exception safe. | ||
| * | ||
| * In general, code should not throw while resolving a [Promise]. Instead, [Promise.reject] should be used. | ||
| * This doesn't match Kotlin's error style, which uses exception. The helper methods in this class, provide such | ||
| * convenience, they can only be called in a context that will catch any Exception and reject the [Promise]. | ||
| * | ||
| */ | ||
| abstract class BitmovinBaseModule( | ||
| protected val context: ReactApplicationContext, | ||
| ) : ReactContextBaseJavaModule(context) { | ||
| /** | ||
| * Runs [block] on the UI thread with [UIManagerModule.addUIBlock] and [TPromise.resolve] [this] with | ||
| * its return value. If [block] throws, [Promise.reject] [this] with the [Throwable]. | ||
| */ | ||
| protected inline fun <T, R : T> TPromise<T>.resolveOnUiThread( | ||
| crossinline block: RejectPromiseOnExceptionBlock.() -> R, | ||
| ) { | ||
| val uiManager = runAndRejectOnException { uiManager } ?: return | ||
| uiManager.addUIBlock { | ||
| resolveOnCurrentThread { block() } | ||
| } | ||
| } | ||
|
|
||
| protected val RejectPromiseOnExceptionBlock.playerModule: PlayerModule get() = context.playerModule | ||
| ?: throw IllegalArgumentException("PlayerModule not found") | ||
|
|
||
| protected val RejectPromiseOnExceptionBlock.uiManager: UIManagerModule get() = context.uiManagerModule | ||
| ?: throw IllegalStateException("UIManager not found") | ||
|
|
||
| protected val RejectPromiseOnExceptionBlock.sourceModule: SourceModule get() = context.sourceModule | ||
| ?: throw IllegalStateException("SourceModule not found") | ||
|
|
||
| protected val RejectPromiseOnExceptionBlock.offlineModule: OfflineModule get() = context.offlineModule | ||
| ?: throw IllegalStateException("OfflineModule not found") | ||
|
|
||
| protected val RejectPromiseOnExceptionBlock.drmModule: DrmModule get() = context.drmModule | ||
| ?: throw IllegalStateException("DrmModule not found") | ||
|
|
||
| fun RejectPromiseOnExceptionBlock.getPlayer( | ||
| nativeId: NativeId, | ||
| playerModule: PlayerModule = this.playerModule, | ||
| ): Player = playerModule.getPlayerOrNull(nativeId) ?: throw IllegalArgumentException("Invalid PlayerId $nativeId") | ||
|
|
||
| fun RejectPromiseOnExceptionBlock.getSource( | ||
| nativeId: NativeId, | ||
| sourceModule: SourceModule = this.sourceModule, | ||
| ): Source = sourceModule.getSourceOrNull(nativeId) ?: throw IllegalArgumentException("Invalid SourceId $nativeId") | ||
| } | ||
|
|
||
| /** Run [block], returning it's return value. If [block] throws, [Promise.reject] [this] and return null. */ | ||
| inline fun <T, R> TPromise<T>.runAndRejectOnException(block: RejectPromiseOnExceptionBlock.() -> R): R? = try { | ||
| RejectPromiseOnExceptionBlock.block() | ||
| } catch (e: Exception) { | ||
| reject(e) | ||
| null | ||
| } | ||
|
|
||
| /** | ||
| * [TPromise.resolve] [this] with [block] return value. | ||
| * If [block] throws, [Promise.reject] [this] with the [Throwable]. | ||
| */ | ||
| inline fun <T> TPromise<T>.resolveOnCurrentThread( | ||
| crossinline block: RejectPromiseOnExceptionBlock.() -> T, | ||
| ): Unit = runAndRejectOnException { this@resolveOnCurrentThread.resolve(block()) } ?: Unit | ||
|
|
||
| /** Receiver of code that can safely throw when resolving a [Promise]. */ | ||
| object RejectPromiseOnExceptionBlock | ||
|
|
||
| /** Compile time wrapper for Promises to type check the resolved type [T]. */ | ||
| @JvmInline | ||
| value class TPromise<T>(val promise: Promise) { | ||
| // Promise only support built-in types. Functions that return [Unit] must resolve to `null`. | ||
| fun resolve(value: T): Unit = promise.resolve(value.takeUnless { it is Unit }) | ||
| fun reject(throwable: Throwable) { | ||
| Log.e(MODULE_NAME, "Failed to execute Bitmovin method", throwable) | ||
| promise.reject(throwable) | ||
| } | ||
| } | ||
|
|
||
| inline val Promise.int get() = TPromise<Int>(this) | ||
| inline val Promise.unit get() = TPromise<Unit>(this) | ||
| inline val Promise.string get() = TPromise<String>(this) | ||
| inline val Promise.double get() = TPromise<Double>(this) | ||
| inline val Promise.float get() = TPromise<Float>(this) | ||
| inline val Promise.bool get() = TPromise<Boolean>(this) | ||
| inline val Promise.map get() = TPromise<ReadableMap>(this) | ||
| inline val Promise.array get() = TPromise<ReadableArray>(this) | ||
| inline val <T> TPromise<T>.nullable get() = TPromise<T?>(promise) |
44 changes: 14 additions & 30 deletions
44
android/src/main/java/com/bitmovin/player/reactnative/BitmovinCastManagerModule.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.