test: Add failing tests for clearing optional view props (#1184)

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/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

packages/react-native-nitro-test/nitrogen/generated/ios/NitroTest-Swift-Cxx-Bridge.hpp

@@ -2142,5 +2142,35 @@ namespace margelo::nitro::test::bridge::swift {
   inline Result_std__variant_std__shared_ptr_margelo__nitro__test__external__HybridSomeExternalObjectSpec___std__string__ create_Result_std__variant_std__shared_ptr_margelo__nitro__test__external__HybridSomeExternalObjectSpec___std__string__(const std::exception_ptr& error) noexcept {
     return Result<std::variant<std::shared_ptr<margelo::nitro::test::external::HybridSomeExternalObjectSpec>, std::string>>::withError(error);
   }
+
+  // pragma MARK: std::optional<std::variant<nitro::NullType, std::string>>
+  /**
+   * Specialized version of `std::optional<std::variant<nitro::NullType, std::string>>`.
+   */
+  using std__optional_std__variant_nitro__NullType__std__string__ = std::optional<std::variant<nitro::NullType, std::string>>;
+  inline std::optional<std::variant<nitro::NullType, std::string>> create_std__optional_std__variant_nitro__NullType__std__string__(const std::variant<nitro::NullType, std::string>& value) noexcept {
+    return std::optional<std::variant<nitro::NullType, std::string>>(value);
+  }
+  inline bool has_value_std__optional_std__variant_nitro__NullType__std__string__(const std::optional<std::variant<nitro::NullType, std::string>>& optional) noexcept {
+    return optional.has_value();
+  }
+  inline std::variant<nitro::NullType, std::string> get_std__optional_std__variant_nitro__NullType__std__string__(const std::optional<std::variant<nitro::NullType, std::string>>& optional) noexcept {
+    return optional.value();
+  }
+
+  // pragma MARK: std::optional<std::function<void()>>
+  /**
+   * Specialized version of `std::optional<std::function<void()>>`.
+   */
+  using std__optional_std__function_void____ = std::optional<std::function<void()>>;
+  inline std::optional<std::function<void()>> create_std__optional_std__function_void____(const std::function<void()>& value) noexcept {
+    return std::optional<std::function<void()>>(value);
+  }
+  inline bool has_value_std__optional_std__function_void____(const std::optional<std::function<void()>>& optional) noexcept {
+    return optional.has_value();
+  }
+  inline std::function<void()> get_std__optional_std__function_void____(const std::optional<std::function<void()>>& optional) noexcept {
+    return optional.value();
+  }
 
 } // namespace margelo::nitro::test::bridge::swift