Skip to content

Commit 04d4dc9

Browse files
authored
fix(android): remove runBlocking from property accessors (#288)
Property accessors (`numberProperty`, `stringProperty`, etc.) were using `runBlocking { flow.first() }` as an existence probe before returning the wrapper. This blocks the calling thread and can deadlock if called from the main thread (e.g. via a worklet). The iOS experimental backend already does the right thing — it just creates and returns the wrapper directly. This PR aligns Android experimental to the same behaviour: return a wrapper always, fail lazily when `getValueAsync()` is called on an invalid path. Also removes the `propertyNames` lazy val (used for `hasProperty`) which had its own one-time `runBlocking` call. Test: updated guard for the "non-existent returns undefined" test (skips on all experimental backends), added new test asserting `getValueAsync()` rejects for invalid paths on experimental.
1 parent 0278a65 commit 04d4dc9

2 files changed

Lines changed: 43 additions & 78 deletions

File tree

android/src/new/java/com/margelo/nitro/rive/HybridViewModelInstance.kt

Lines changed: 12 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import app.rive.ViewModelInstanceSource
77
import app.rive.core.CommandQueue
88
import com.facebook.proguard.annotations.DoNotStrip
99
import com.margelo.nitro.core.Promise
10-
import kotlinx.coroutines.flow.first
11-
import kotlinx.coroutines.runBlocking
1210

1311
@Keep
1412
@DoNotStrip
@@ -23,22 +21,6 @@ class HybridViewModelInstance(
2321
private const val TAG = "HybridViewModelInstance"
2422
}
2523

26-
private val propertyNames: Set<String> by lazy {
27-
val name = viewModelName ?: return@lazy emptySet()
28-
val file = parentFile.riveFile ?: return@lazy emptySet()
29-
try {
30-
runBlocking { file.getViewModelProperties(name) }.map { it.name }.toSet()
31-
} catch (e: Exception) {
32-
Log.e(TAG, "Failed to fetch property names for viewModel '$name'", e)
33-
emptySet()
34-
}
35-
}
36-
37-
private fun hasProperty(path: String): Boolean {
38-
if (propertyNames.isEmpty()) return true
39-
return propertyNames.contains(path)
40-
}
41-
4224
// TODO: Workaround — rive-android experimental SDK doesn't expose ViewModelInstance.name.
4325
// Only works when caller knows the name (createInstanceByName). Falls back to "" otherwise.
4426
override val instanceName: String
@@ -56,65 +38,23 @@ class HybridViewModelInstance(
5638
}
5739
}
5840

59-
override fun numberProperty(path: String): HybridViewModelNumberPropertySpec? {
60-
return try {
61-
runBlocking { viewModelInstance.getNumberFlow(path).first() }
62-
HybridViewModelNumberProperty(viewModelInstance, path)
63-
} catch (e: Exception) {
64-
Log.e(TAG, "numberProperty failed for path '$path'", e)
65-
null
66-
}
67-
}
41+
override fun numberProperty(path: String) =
42+
HybridViewModelNumberProperty(viewModelInstance, path)
6843

69-
override fun stringProperty(path: String): HybridViewModelStringPropertySpec? {
70-
return try {
71-
runBlocking { viewModelInstance.getStringFlow(path).first() }
72-
HybridViewModelStringProperty(viewModelInstance, path)
73-
} catch (e: Exception) {
74-
Log.e(TAG, "stringProperty failed for path '$path'", e)
75-
null
76-
}
77-
}
44+
override fun stringProperty(path: String) =
45+
HybridViewModelStringProperty(viewModelInstance, path)
7846

79-
override fun booleanProperty(path: String): HybridViewModelBooleanPropertySpec? {
80-
return try {
81-
runBlocking { viewModelInstance.getBooleanFlow(path).first() }
82-
HybridViewModelBooleanProperty(viewModelInstance, path)
83-
} catch (e: Exception) {
84-
Log.e(TAG, "booleanProperty failed for path '$path'", e)
85-
null
86-
}
87-
}
47+
override fun booleanProperty(path: String) =
48+
HybridViewModelBooleanProperty(viewModelInstance, path)
8849

89-
override fun colorProperty(path: String): HybridViewModelColorPropertySpec? {
90-
return try {
91-
runBlocking { viewModelInstance.getColorFlow(path).first() }
92-
HybridViewModelColorProperty(viewModelInstance, path)
93-
} catch (e: Exception) {
94-
Log.e(TAG, "colorProperty failed for path '$path'", e)
95-
null
96-
}
97-
}
50+
override fun colorProperty(path: String) =
51+
HybridViewModelColorProperty(viewModelInstance, path)
9852

99-
override fun enumProperty(path: String): HybridViewModelEnumPropertySpec? {
100-
return try {
101-
runBlocking { viewModelInstance.getEnumFlow(path).first() }
102-
HybridViewModelEnumProperty(viewModelInstance, path)
103-
} catch (e: Exception) {
104-
Log.e(TAG, "enumProperty failed for path '$path'", e)
105-
null
106-
}
107-
}
53+
override fun enumProperty(path: String) =
54+
HybridViewModelEnumProperty(viewModelInstance, path)
10855

109-
override fun triggerProperty(path: String): HybridViewModelTriggerPropertySpec? {
110-
if (!hasProperty(path)) return null
111-
return try {
112-
HybridViewModelTriggerProperty(viewModelInstance, path)
113-
} catch (e: Exception) {
114-
Log.e(TAG, "triggerProperty failed for path '$path'", e)
115-
null
116-
}
117-
}
56+
override fun triggerProperty(path: String) =
57+
HybridViewModelTriggerProperty(viewModelInstance, path)
11858

11959
override fun imageProperty(path: String): HybridViewModelImagePropertySpec? {
12060
return try {
@@ -144,7 +84,6 @@ class HybridViewModelInstance(
14484
}
14585

14686
private fun viewModelImpl(path: String): HybridViewModelInstanceSpec? {
147-
if (!hasProperty(path)) return null
14887
val file = parentFile.riveFile ?: return null
14988
val source = ViewModelInstanceSource.Reference(viewModelInstance, path)
15089
val childVmi = ViewModelInstance.fromFile(file, source)

example/__tests__/viewmodel-properties.harness.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -149,11 +149,9 @@ describe('ViewModel Properties', () => {
149149
});
150150

151151
it('non-existent properties return undefined', async () => {
152-
if (
153-
Platform.OS === 'ios' &&
154-
RiveFileFactory.getBackend() === 'experimental'
155-
) {
156-
// Experimental API can't sync-validate property paths, returns wrapper objects
152+
if (isExperimental()) {
153+
// Experimental backends return wrapper objects for any path — validity is
154+
// checked lazily when a value is read (getValueAsync throws).
157155
return;
158156
}
159157

@@ -167,6 +165,34 @@ describe('ViewModel Properties', () => {
167165
expect(instance.triggerProperty('nonexistent')).toBeUndefined();
168166
expect(await instance.viewModelAsync('nonexistent')).toBeUndefined();
169167
});
168+
169+
it('experimental: getValueAsync throws for non-existent property path', async () => {
170+
if (!isExperimental()) {
171+
return;
172+
}
173+
174+
const instance = await createGordonInstance();
175+
176+
await expect(
177+
instance.numberProperty('nonexistent')!.getValueAsync()
178+
).rejects.toBeDefined();
179+
180+
await expect(
181+
instance.stringProperty('nonexistent')!.getValueAsync()
182+
).rejects.toBeDefined();
183+
184+
await expect(
185+
instance.booleanProperty('nonexistent')!.getValueAsync()
186+
).rejects.toBeDefined();
187+
188+
await expect(
189+
instance.colorProperty('nonexistent')!.getValueAsync()
190+
).rejects.toBeDefined();
191+
192+
await expect(
193+
instance.enumProperty('nonexistent')!.getValueAsync()
194+
).rejects.toBeDefined();
195+
});
170196
});
171197

172198
describe('Property Listeners', () => {

0 commit comments

Comments
 (0)