fix: Fix clearing optional view props crashing with "Value is null"

example/__tests__/views.harness.tsx

@@ -0,0 +1,202 @@
+import * as React from 'react'
+import {
+  describe,
+  it,
+  expect,
+  render,
+  cleanup,
+  waitFor,
+  afterEach,
+} from 'react-native-harness'
+import { callback } from 'react-native-nitro-modules'
+import { TestView, type TestViewRef } from 'react-native-nitro-test'
+
+type TestViewElementProps = React.ComponentProps<typeof TestView>
+type OptionalTestViewProps = Partial<
+  Pick<
+    TestViewElementProps,
+    'optionalString' | 'nullableString' | 'optionalCallback'
+  >
+>
+
+// Prop-parsing errors are thrown during React's commit and are only
+// observable through an error boundary - the test-runner's own boundary
+// would swallow them and remount, masking the failure.
+class CatchRenderErrors extends React.Component<
+  { children: React.ReactNode; onError: (error: unknown) => void },
+  { hasError: boolean }
+> {
+  state = { hasError: false }
+  static getDerivedStateFromError() {
+    return { hasError: true }
+  }
+  componentDidCatch(error: unknown) {
+    this.props.onError(error)
+  }
+  render() {
+    return this.state.hasError ? null : this.props.children
+  }
+}
+
+// React encodes "prop was removed" as `null` (not `undefined`) in the Fabric
+// update payload, which used to crash Nitro's prop parsing:
+// https://github.com/mrousavy/nitro/issues/1184
+describe('TestView optional props', () => {
+  afterEach(() => {
+    cleanup()
+  })
+
+  async function renderTestView(initialProps: OptionalTestViewProps): Promise<{
+    view: TestViewRef
+    errors: unknown[]
+    update: (newProps: OptionalTestViewProps) => void
+    mountCount: () => number
+  }> {
+    const refs: TestViewRef[] = []
+    const errors: unknown[] = []
+    const hybridRef = callback((ref: TestViewRef) => {
+      refs.push(ref)
+    })
+    let setPropsFromTest: ((props: OptionalTestViewProps) => void) | undefined
+    function Wrapper(): React.ReactElement {
+      const [optionalProps, setOptionalProps] = React.useState(initialProps)
+      setPropsFromTest = setOptionalProps
+      return (
+        <TestView
+          hybridRef={hybridRef}
+          isBlue={false}
+          hasBeenCalled={false}
+          colorScheme="light"
+          someCallback={callback(() => {})}
+          {...optionalProps}
+        />
+      )
+    }
+
+    await render(
+      <CatchRenderErrors onError={(error) => errors.push(error)}>
+        <Wrapper />
+      </CatchRenderErrors>
+    )
+    await waitFor(() => expect(refs.length).toBeGreaterThan(0))
+    return {
+      view: refs[0]!,
+      errors,
+      update: (newProps) => setPropsFromTest!(newProps),
+      mountCount: () => refs.length,
+    }
+  }
+
+  function expectNoRenderErrors(errors: unknown[]): void {
+    if (errors.length > 0) {
+      throw new Error(
+        `Caught render error(s): ${errors.map((e) => String(e)).join('\n')}`
+      )
+    }
+  }
+
+  // Resolves once the prop update landed OR a render error was caught, so a
+  // crashing commit fails fast in `expectNoRenderErrors` with the real error
+  // instead of timing out here.
+  async function waitForUpdateOrError(
+    errors: unknown[],
+    hasUpdated: () => boolean
+  ): Promise<void> {
+    await waitFor(() => {
+      if (errors.length === 0 && !hasUpdated()) {
+        throw new Error('prop update has not reached the native view yet')
+      }
+    })
+  }
+
+  it('renders with optional props omitted', async () => {
+    const { view, errors } = await renderTestView({})
+    expect(view.optionalString).toBeUndefined()
+    expect(view.optionalCallback).toBeUndefined()
+    expectNoRenderErrors(errors)
+  })
+
+  it('clears optionalString when the prop is removed', async () => {
+    const { view, errors, update, mountCount } = await renderTestView({
+      optionalString: 'hello',
+    })
+    await waitFor(() => expect(view.optionalString).toBe('hello'))
+
+    update({})
+    await waitForUpdateOrError(errors, () => view.optionalString === undefined)
+    expectNoRenderErrors(errors)
+    expect(view.optionalString).toBeUndefined()
+    expect(mountCount()).toBe(1)
+  })
+
+  it('clears optionalString when the prop is set to undefined', async () => {
+    const { view, errors, update, mountCount } = await renderTestView({
+      optionalString: 'hello',
+    })
+    await waitFor(() => expect(view.optionalString).toBe('hello'))
+
+    update({ optionalString: undefined })
+    await waitForUpdateOrError(errors, () => view.optionalString === undefined)
+    expectNoRenderErrors(errors)
+    expect(view.optionalString).toBeUndefined()
+    expect(mountCount()).toBe(1)
+  })
+
+  it('can re-set optionalString after it was cleared', async () => {
+    const { view, errors, update } = await renderTestView({
+      optionalString: 'first',
+    })
+    await waitFor(() => expect(view.optionalString).toBe('first'))
+
+    update({})
+    await waitForUpdateOrError(errors, () => view.optionalString === undefined)
+    update({ optionalString: 'second' })
+    await waitForUpdateOrError(errors, () => view.optionalString === 'second')
+    expectNoRenderErrors(errors)
+    expect(view.optionalString).toBe('second')
+  })
+
+  it('clears optionalCallback when the prop is removed', async () => {
+    const { view, errors, update, mountCount } = await renderTestView({
+      optionalCallback: callback(() => {}),
+    })
+    await waitFor(() => expect(view.optionalCallback).toBeDefined())
+
+    update({})
+    await waitForUpdateOrError(
+      errors,
+      () => view.optionalCallback === undefined
+    )
+    expectNoRenderErrors(errors)
+    expect(view.optionalCallback).toBeUndefined()
+    expect(mountCount()).toBe(1)
+  })
+
+  it('keeps explicit null for props that model null (nullableString)', async () => {
+    const { view, errors, update } = await renderTestView({
+      nullableString: 'hello',
+    })
+    await waitFor(() => expect(view.nullableString).toBe('hello'))
+
+    // `string | null` props explicitly model `null`, so an explicit `null`
+    // value must still arrive as `null` - not be swallowed into `undefined`.
+    update({ nullableString: null })
+    await waitForUpdateOrError(errors, () => view.nullableString === null)
+    expectNoRenderErrors(errors)
+    expect(view.nullableString).toBeNull()
+  })
+
+  it('delivers null when nullableString is removed', async () => {
+    const { view, errors, update } = await renderTestView({
+      nullableString: 'hello',
+    })
+    await waitFor(() => expect(view.nullableString).toBe('hello'))
+
+    // React encodes removal as `null` and native cannot tell it apart from an
+    // explicit `null` - since `string | null` models null, it arrives as-is.
+    update({})
+    await waitForUpdateOrError(errors, () => view.nullableString === null)
+    expectNoRenderErrors(errors)
+    expect(view.nullableString).toBeNull()
+  })
+})

packages/nitrogen/src/views/CppHybridViewComponent.ts

@@ -202,7 +202,8 @@ namespace ${namespace} {
       // Due to a React limitation, functions cannot be passed to native directly,
       // because RN converts them to booleans (`true`). Nitro knows this and just
       // wraps functions as objects - the original function is stored in `f`.
-      valueConversion = `value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f"))`
+      // React encodes "prop was removed" as `null`, so only unwrap actual objects.
+      valueConversion = `(value.isNull() || value.isUndefined()) ? jsi::Value::undefined() : value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f"))`
     }
 
     propInitializers.push(

packages/react-native-nitro-modules/cpp/views/CachedProp.hpp

@@ -46,14 +46,31 @@ struct CachedProp {
       // jsi::Value hasn't changed - no need to convert it again!
       return oldProp;
     }
-    T converted = JSIConverter<T>::fromJSI(runtime, value);
+    T converted = convertFromJSI(runtime, value);
     BorrowingReference<jsi::Value> cached;
     {
       JSICacheReference cache = JSICache::getOrCreateCache(runtime);
       cached = cache.makeShared(jsi::Value(runtime, value));
     }
     return CachedProp<T>(std::move(converted), std::move(cached));
   }
+
+private:
+  static T convertFromJSI(jsi::Runtime& runtime, const jsi::Value& value) {
+    if (value.isNull() && !JSIConverter<T>::canConvert(runtime, value)) {
+      // React encodes removed (or explicitly `undefined`) props as `null` - unless T
+      // explicitly models null (e.g. `string | null`), treat it as `undefined`.
+      // See https://github.com/mrousavy/nitro/issues/1184 and
+      // https://github.com/facebook/react-native/blob/v0.85.3/packages/react-native/Libraries/ReactNative/ReactFabricPublicInstance/ReactNativeAttributePayload.js#L270-L277
+      jsi::Value undefined = jsi::Value::undefined();
+      if (JSIConverter<T>::canConvert(runtime, undefined)) {
+        return JSIConverter<T>::fromJSI(runtime, undefined);
+      }
+      // T can't represent "no value" either (required prop) - fall through so
+      // the error reports the actual (null) value.
+    }
+    return JSIConverter<T>::fromJSI(runtime, value);
+  }
 };
 
 } // namespace margelo::nitro

packages/react-native-nitro-test/android/src/main/java/com/margelo/nitro/test/HybridTestView.kt

@@ -24,6 +24,9 @@ class HybridTestView(
   override var hasBeenCalled: Boolean = false
   override var colorScheme: ColorScheme = ColorScheme.LIGHT
   override var someCallback: () -> Unit = {}
+  override var optionalString: String? = null
+  override var nullableString: Variant_NullType_String? = null
+  override var optionalCallback: (() -> Unit)? = null
 
   // Methods
   override fun someMethod() {

packages/react-native-nitro-test/ios/HybridTestView.swift

@@ -21,6 +21,9 @@ class HybridTestView: HybridTestViewSpec {
   var hasBeenCalled: Bool = false
   var colorScheme: ColorScheme = .light
   var someCallback: () -> Void = {}
+  var optionalString: String? = nil
+  var nullableString: Variant_NullType_String? = nil
+  var optionalCallback: (() -> Void)? = nil
 
   // Methods
   func someMethod() throws {

packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.cpp

@@ -15,6 +15,12 @@ namespace margelo::nitro::test { enum class ColorScheme; }
 #include <functional>
 #include "JFunc_void.hpp"
 #include <NitroModules/JNICallable.hpp>
+#include <string>
+#include <optional>
+#include <NitroModules/Null.hpp>
+#include <variant>
+#include "JVariant_NullType_String.hpp"
+#include <NitroModules/JNull.hpp>
 
 namespace margelo::nitro::test {
 
@@ -90,6 +96,41 @@ namespace margelo::nitro::test {
     static const auto method = _javaPart->javaClassStatic()->getMethod<void(jni::alias_ref<JFunc_void::javaobject> /* someCallback */)>("setSomeCallback_cxx");
     method(_javaPart, JFunc_void_cxx::fromCpp(someCallback));
   }
+  std::optional<std::string> JHybridTestViewSpec::getOptionalString() {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<jni::local_ref<jni::JString>()>("getOptionalString");
+    auto __result = method(_javaPart);
+    return __result != nullptr ? std::make_optional(__result->toStdString()) : std::nullopt;
+  }
+  void JHybridTestViewSpec::setOptionalString(const std::optional<std::string>& optionalString) {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<void(jni::alias_ref<jni::JString> /* optionalString */)>("setOptionalString");
+    method(_javaPart, optionalString.has_value() ? jni::make_jstring(optionalString.value()) : nullptr);
+  }
+  std::optional<std::variant<nitro::NullType, std::string>> JHybridTestViewSpec::getNullableString() {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<jni::local_ref<JVariant_NullType_String>()>("getNullableString");
+    auto __result = method(_javaPart);
+    return __result != nullptr ? std::make_optional(__result->toCpp()) : std::nullopt;
+  }
+  void JHybridTestViewSpec::setNullableString(const std::optional<std::variant<nitro::NullType, std::string>>& nullableString) {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<void(jni::alias_ref<JVariant_NullType_String> /* nullableString */)>("setNullableString");
+    method(_javaPart, nullableString.has_value() ? JVariant_NullType_String::fromCpp(nullableString.value()) : nullptr);
+  }
+  std::optional<std::function<void()>> JHybridTestViewSpec::getOptionalCallback() {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<jni::local_ref<JFunc_void::javaobject>()>("getOptionalCallback_cxx");
+    auto __result = method(_javaPart);
+    return __result != nullptr ? std::make_optional([&]() -> std::function<void()> {
+      if (__result->isInstanceOf(JFunc_void_cxx::javaClassStatic())) [[likely]] {
+        auto downcast = jni::static_ref_cast<JFunc_void_cxx::javaobject>(__result);
+        return downcast->cthis()->getFunction();
+      } else {
+        auto __resultRef = jni::make_global(__result);
+        return JNICallable<JFunc_void, void()>(std::move(__resultRef));
+      }
+    }()) : std::nullopt;
+  }
+  void JHybridTestViewSpec::setOptionalCallback(const std::optional<std::function<void()>>& optionalCallback) {
+    static const auto method = _javaPart->javaClassStatic()->getMethod<void(jni::alias_ref<JFunc_void::javaobject> /* optionalCallback */)>("setOptionalCallback_cxx");
+    method(_javaPart, optionalCallback.has_value() ? JFunc_void_cxx::fromCpp(optionalCallback.value()) : nullptr);
+  }
 
   // Methods
   void JHybridTestViewSpec::someMethod() {

packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.hpp

@@ -58,6 +58,12 @@ namespace margelo::nitro::test {
     void setColorScheme(ColorScheme colorScheme) override;
     std::function<void()> getSomeCallback() override;
     void setSomeCallback(const std::function<void()>& someCallback) override;
+    std::optional<std::string> getOptionalString() override;
+    void setOptionalString(const std::optional<std::string>& optionalString) override;
+    std::optional<std::variant<nitro::NullType, std::string>> getNullableString() override;
+    void setNullableString(const std::optional<std::variant<nitro::NullType, std::string>>& nullableString) override;
+    std::optional<std::function<void()>> getOptionalCallback() override;
+    void setOptionalCallback(const std::optional<std::function<void()>>& optionalCallback) override;
 
   public:
     // Methods

packages/react-native-nitro-test/nitrogen/generated/android/c++/views/JHybridTestViewStateUpdater.cpp

@@ -53,6 +53,18 @@ void JHybridTestViewStateUpdater::updateViewProps(jni::alias_ref<jni::JClass> /*
     hybridView->setSomeCallback(props->someCallback.value);
     props->someCallback.isDirty = false;
   }
+  if (props->optionalString.isDirty) {
+    hybridView->setOptionalString(props->optionalString.value);
+    props->optionalString.isDirty = false;
+  }
+  if (props->nullableString.isDirty) {
+    hybridView->setNullableString(props->nullableString.value);
+    props->nullableString.isDirty = false;
+  }
+  if (props->optionalCallback.isDirty) {
+    hybridView->setOptionalCallback(props->optionalCallback.value);
+    props->optionalCallback.isDirty = false;
+  }
 
   // Update hybridRef if it changed
   if (props->hybridRef.isDirty) {

packages/react-native-nitro-test/nitrogen/generated/android/kotlin/com/margelo/nitro/test/HybridTestViewSpec.kt

@@ -10,6 +10,7 @@ package com.margelo.nitro.test
 import androidx.annotation.Keep
 import com.facebook.jni.HybridData
 import com.facebook.proguard.annotations.DoNotStrip
+import com.margelo.nitro.core.NullType
 import com.margelo.nitro.core.HybridObject
 import com.margelo.nitro.views.HybridView
 
@@ -57,6 +58,32 @@ abstract class HybridTestViewSpec: HybridView() {
     set(value) {
       someCallback = value
     }
+
+  @get:DoNotStrip
+  @get:Keep
+  @set:DoNotStrip
+  @set:Keep
+  abstract var optionalString: String?
+
+  @get:DoNotStrip
+  @get:Keep
+  @set:DoNotStrip
+  @set:Keep
+  abstract var nullableString: Variant_NullType_String?
+
+  abstract var optionalCallback: (() -> Unit)?
+
+  private var optionalCallback_cxx: Func_void?
+    @Keep
+    @DoNotStrip
+    get() {
+      return optionalCallback?.let { Func_void_java(it) }
+    }
+    @Keep
+    @DoNotStrip
+    set(value) {
+      optionalCallback = value?.let { it }
+    }
 
   // Methods
   @DoNotStrip