From 2fcf067baa786053fe96dd1fce701294203ce742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikl=C3=B3s=20Fazekas?= Date: Wed, 8 Jul 2026 14:04:44 +0200 Subject: [PATCH 1/2] fix: Fix clearing optional view props crashing with "Value is null" --- example/__tests__/views.harness.tsx | 185 ++++++++++++++++++ .../src/views/CppHybridViewComponent.ts | 3 +- .../cpp/views/CachedProp.hpp | 14 +- .../com/margelo/nitro/test/HybridTestView.kt | 3 + .../ios/HybridTestView.swift | 3 + .../android/c++/JHybridTestViewSpec.cpp | 41 ++++ .../android/c++/JHybridTestViewSpec.hpp | 6 + .../c++/views/JHybridTestViewStateUpdater.cpp | 12 ++ .../margelo/nitro/test/HybridTestViewSpec.kt | 27 +++ .../ios/NitroTest-Swift-Cxx-Bridge.hpp | 30 +++ .../ios/c++/HybridTestViewSpecSwift.hpp | 25 +++ .../ios/c++/views/HybridTestViewComponent.mm | 15 ++ .../ios/swift/HybridTestViewSpec.swift | 3 + .../ios/swift/HybridTestViewSpec_cxx.swift | 99 ++++++++++ .../shared/c++/HybridTestViewSpec.cpp | 6 + .../shared/c++/HybridTestViewSpec.hpp | 10 + .../HybridRecyclableTestViewComponent.cpp | 2 +- .../c++/views/HybridTestViewComponent.cpp | 37 +++- .../c++/views/HybridTestViewComponent.hpp | 8 +- .../generated/shared/json/TestViewConfig.json | 3 + .../src/specs/TestView.nitro.ts | 3 + 21 files changed, 529 insertions(+), 6 deletions(-) create mode 100644 example/__tests__/views.harness.tsx diff --git a/example/__tests__/views.harness.tsx b/example/__tests__/views.harness.tsx new file mode 100644 index 0000000000..f1be5ac1b6 --- /dev/null +++ b/example/__tests__/views.harness.tsx @@ -0,0 +1,185 @@ +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 +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 ( + {})} + {...optionalProps} + /> + ) + } + + await render( + errors.push(error)}> + + + ) + 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')}` + ) + } + } + + 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 waitFor(() => + expect(errors.length > 0 || view.optionalString === undefined).toBe(true) + ) + 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 waitFor(() => + expect(errors.length > 0 || view.optionalString === undefined).toBe(true) + ) + 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 waitFor(() => + expect(errors.length > 0 || view.optionalString === undefined).toBe(true) + ) + update({ optionalString: 'second' }) + await waitFor(() => + expect(errors.length > 0 || view.optionalString === 'second').toBe(true) + ) + 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 waitFor(() => + expect(errors.length > 0 || view.optionalCallback === undefined).toBe( + true + ) + ) + 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 waitFor(() => + expect(errors.length > 0 || view.nullableString === null).toBe(true) + ) + expectNoRenderErrors(errors) + expect(view.nullableString).toBeNull() + }) +}) diff --git a/packages/nitrogen/src/views/CppHybridViewComponent.ts b/packages/nitrogen/src/views/CppHybridViewComponent.ts index 9e9fd47f18..7ceb7022dc 100644 --- a/packages/nitrogen/src/views/CppHybridViewComponent.ts +++ b/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( diff --git a/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp b/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp index 744324d70d..839b023c98 100644 --- a/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp +++ b/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp @@ -46,7 +46,7 @@ struct CachedProp { // jsi::Value hasn't changed - no need to convert it again! return oldProp; } - T converted = JSIConverter::fromJSI(runtime, value); + T converted = convertFromJSI(runtime, value); BorrowingReference cached; { JSICacheReference cache = JSICache::getOrCreateCache(runtime); @@ -54,6 +54,18 @@ struct CachedProp { } return CachedProp(std::move(converted), std::move(cached)); } + +private: + static T convertFromJSI(jsi::Runtime& runtime, const jsi::Value& value) { + if (value.isNull() && !JSIConverter::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 + return JSIConverter::fromJSI(runtime, jsi::Value::undefined()); + } + return JSIConverter::fromJSI(runtime, value); + } }; } // namespace margelo::nitro diff --git a/packages/react-native-nitro-test/android/src/main/java/com/margelo/nitro/test/HybridTestView.kt b/packages/react-native-nitro-test/android/src/main/java/com/margelo/nitro/test/HybridTestView.kt index a27e6ccfe0..c2b41bcb07 100644 --- a/packages/react-native-nitro-test/android/src/main/java/com/margelo/nitro/test/HybridTestView.kt +++ b/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() { diff --git a/packages/react-native-nitro-test/ios/HybridTestView.swift b/packages/react-native-nitro-test/ios/HybridTestView.swift index 0fffdb9bd4..7e1ad73588 100644 --- a/packages/react-native-nitro-test/ios/HybridTestView.swift +++ b/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 { diff --git a/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.cpp b/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.cpp index 7a612c4af4..d7a8ecee49 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.cpp +++ b/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.cpp @@ -15,6 +15,12 @@ namespace margelo::nitro::test { enum class ColorScheme; } #include #include "JFunc_void.hpp" #include +#include +#include +#include +#include +#include "JVariant_NullType_String.hpp" +#include namespace margelo::nitro::test { @@ -90,6 +96,41 @@ namespace margelo::nitro::test { static const auto method = _javaPart->javaClassStatic()->getMethod /* someCallback */)>("setSomeCallback_cxx"); method(_javaPart, JFunc_void_cxx::fromCpp(someCallback)); } + std::optional JHybridTestViewSpec::getOptionalString() { + static const auto method = _javaPart->javaClassStatic()->getMethod()>("getOptionalString"); + auto __result = method(_javaPart); + return __result != nullptr ? std::make_optional(__result->toStdString()) : std::nullopt; + } + void JHybridTestViewSpec::setOptionalString(const std::optional& optionalString) { + static const auto method = _javaPart->javaClassStatic()->getMethod /* optionalString */)>("setOptionalString"); + method(_javaPart, optionalString.has_value() ? jni::make_jstring(optionalString.value()) : nullptr); + } + std::optional> JHybridTestViewSpec::getNullableString() { + static const auto method = _javaPart->javaClassStatic()->getMethod()>("getNullableString"); + auto __result = method(_javaPart); + return __result != nullptr ? std::make_optional(__result->toCpp()) : std::nullopt; + } + void JHybridTestViewSpec::setNullableString(const std::optional>& nullableString) { + static const auto method = _javaPart->javaClassStatic()->getMethod /* nullableString */)>("setNullableString"); + method(_javaPart, nullableString.has_value() ? JVariant_NullType_String::fromCpp(nullableString.value()) : nullptr); + } + std::optional> JHybridTestViewSpec::getOptionalCallback() { + static const auto method = _javaPart->javaClassStatic()->getMethod()>("getOptionalCallback_cxx"); + auto __result = method(_javaPart); + return __result != nullptr ? std::make_optional([&]() -> std::function { + if (__result->isInstanceOf(JFunc_void_cxx::javaClassStatic())) [[likely]] { + auto downcast = jni::static_ref_cast(__result); + return downcast->cthis()->getFunction(); + } else { + auto __resultRef = jni::make_global(__result); + return JNICallable(std::move(__resultRef)); + } + }()) : std::nullopt; + } + void JHybridTestViewSpec::setOptionalCallback(const std::optional>& optionalCallback) { + static const auto method = _javaPart->javaClassStatic()->getMethod /* optionalCallback */)>("setOptionalCallback_cxx"); + method(_javaPart, optionalCallback.has_value() ? JFunc_void_cxx::fromCpp(optionalCallback.value()) : nullptr); + } // Methods void JHybridTestViewSpec::someMethod() { diff --git a/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.hpp b/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.hpp index 715471554b..7178299dcf 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/android/c++/JHybridTestViewSpec.hpp +++ b/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 getSomeCallback() override; void setSomeCallback(const std::function& someCallback) override; + std::optional getOptionalString() override; + void setOptionalString(const std::optional& optionalString) override; + std::optional> getNullableString() override; + void setNullableString(const std::optional>& nullableString) override; + std::optional> getOptionalCallback() override; + void setOptionalCallback(const std::optional>& optionalCallback) override; public: // Methods diff --git a/packages/react-native-nitro-test/nitrogen/generated/android/c++/views/JHybridTestViewStateUpdater.cpp b/packages/react-native-nitro-test/nitrogen/generated/android/c++/views/JHybridTestViewStateUpdater.cpp index fb65848189..4b64a48d3c 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/android/c++/views/JHybridTestViewStateUpdater.cpp +++ b/packages/react-native-nitro-test/nitrogen/generated/android/c++/views/JHybridTestViewStateUpdater.cpp @@ -53,6 +53,18 @@ void JHybridTestViewStateUpdater::updateViewProps(jni::alias_ref /* 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) { diff --git a/packages/react-native-nitro-test/nitrogen/generated/android/kotlin/com/margelo/nitro/test/HybridTestViewSpec.kt b/packages/react-native-nitro-test/nitrogen/generated/android/kotlin/com/margelo/nitro/test/HybridTestViewSpec.kt index f144d544a3..d5f36a0f59 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/android/kotlin/com/margelo/nitro/test/HybridTestViewSpec.kt +++ b/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 diff --git a/packages/react-native-nitro-test/nitrogen/generated/ios/NitroTest-Swift-Cxx-Bridge.hpp b/packages/react-native-nitro-test/nitrogen/generated/ios/NitroTest-Swift-Cxx-Bridge.hpp index 52d5cce021..4dc2dc011c 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/ios/NitroTest-Swift-Cxx-Bridge.hpp +++ b/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::string>>::withError(error); } + + // pragma MARK: std::optional> + /** + * Specialized version of `std::optional>`. + */ + using std__optional_std__variant_nitro__NullType__std__string__ = std::optional>; + inline std::optional> create_std__optional_std__variant_nitro__NullType__std__string__(const std::variant& value) noexcept { + return std::optional>(value); + } + inline bool has_value_std__optional_std__variant_nitro__NullType__std__string__(const std::optional>& optional) noexcept { + return optional.has_value(); + } + inline std::variant get_std__optional_std__variant_nitro__NullType__std__string__(const std::optional>& optional) noexcept { + return optional.value(); + } + + // pragma MARK: std::optional> + /** + * Specialized version of `std::optional>`. + */ + using std__optional_std__function_void____ = std::optional>; + inline std::optional> create_std__optional_std__function_void____(const std::function& value) noexcept { + return std::optional>(value); + } + inline bool has_value_std__optional_std__function_void____(const std::optional>& optional) noexcept { + return optional.has_value(); + } + inline std::function get_std__optional_std__function_void____(const std::optional>& optional) noexcept { + return optional.value(); + } } // namespace margelo::nitro::test::bridge::swift diff --git a/packages/react-native-nitro-test/nitrogen/generated/ios/c++/HybridTestViewSpecSwift.hpp b/packages/react-native-nitro-test/nitrogen/generated/ios/c++/HybridTestViewSpecSwift.hpp index 986c483ac2..edbcf39e79 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/ios/c++/HybridTestViewSpecSwift.hpp +++ b/packages/react-native-nitro-test/nitrogen/generated/ios/c++/HybridTestViewSpecSwift.hpp @@ -17,6 +17,10 @@ namespace margelo::nitro::test { enum class ColorScheme; } #include "ColorScheme.hpp" #include +#include +#include +#include +#include #include "NitroTest-Swift-Cxx-Umbrella.hpp" @@ -90,6 +94,27 @@ namespace margelo::nitro::test { inline void setSomeCallback(const std::function& someCallback) noexcept override { _swiftPart.setSomeCallback(someCallback); } + inline std::optional getOptionalString() noexcept override { + auto __result = _swiftPart.getOptionalString(); + return __result; + } + inline void setOptionalString(const std::optional& optionalString) noexcept override { + _swiftPart.setOptionalString(optionalString); + } + inline std::optional> getNullableString() noexcept override { + auto __result = _swiftPart.getNullableString(); + return __result; + } + inline void setNullableString(const std::optional>& nullableString) noexcept override { + _swiftPart.setNullableString(nullableString); + } + inline std::optional> getOptionalCallback() noexcept override { + auto __result = _swiftPart.getOptionalCallback(); + return __result; + } + inline void setOptionalCallback(const std::optional>& optionalCallback) noexcept override { + _swiftPart.setOptionalCallback(optionalCallback); + } public: // Methods diff --git a/packages/react-native-nitro-test/nitrogen/generated/ios/c++/views/HybridTestViewComponent.mm b/packages/react-native-nitro-test/nitrogen/generated/ios/c++/views/HybridTestViewComponent.mm index 191b76da7a..95c2963ebf 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/ios/c++/views/HybridTestViewComponent.mm +++ b/packages/react-native-nitro-test/nitrogen/generated/ios/c++/views/HybridTestViewComponent.mm @@ -99,6 +99,21 @@ - (void) updateProps:(const std::shared_ptr&)props swiftPart.setSomeCallback(newViewProps.someCallback.value); newViewProps.someCallback.isDirty = false; } + // optionalString: optional + if (newViewProps.optionalString.isDirty) { + swiftPart.setOptionalString(newViewProps.optionalString.value); + newViewProps.optionalString.isDirty = false; + } + // nullableString: optional + if (newViewProps.nullableString.isDirty) { + swiftPart.setNullableString(newViewProps.nullableString.value); + newViewProps.nullableString.isDirty = false; + } + // optionalCallback: optional + if (newViewProps.optionalCallback.isDirty) { + swiftPart.setOptionalCallback(newViewProps.optionalCallback.value); + newViewProps.optionalCallback.isDirty = false; + } swiftPart.afterUpdate(); diff --git a/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec.swift b/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec.swift index 7f05d599a5..a50650dbbd 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec.swift +++ b/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec.swift @@ -14,6 +14,9 @@ public protocol HybridTestViewSpec_protocol: HybridObject, HybridView { var hasBeenCalled: Bool { get set } var colorScheme: ColorScheme { get set } var someCallback: () -> Void { get set } + var optionalString: String? { get set } + var nullableString: Variant_NullType_String? { get set } + var optionalCallback: (() -> Void)? { get set } // Methods func someMethod() throws -> Void diff --git a/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec_cxx.swift b/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec_cxx.swift index 4c0b9731e4..9ddce31b77 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec_cxx.swift +++ b/packages/react-native-nitro-test/nitrogen/generated/ios/swift/HybridTestViewSpec_cxx.swift @@ -172,6 +172,105 @@ open class HybridTestViewSpec_cxx { }() } } + + public final var optionalString: bridge.std__optional_std__string_ { + @inline(__always) + get { + return { () -> bridge.std__optional_std__string_ in + if let __unwrappedValue = self.__implementation.optionalString { + return bridge.create_std__optional_std__string_(std.string(__unwrappedValue)) + } else { + return .init() + } + }() + } + @inline(__always) + set { + self.__implementation.optionalString = { () -> String? in + if bridge.has_value_std__optional_std__string_(newValue) { + let __unwrapped = bridge.get_std__optional_std__string_(newValue) + return String(__unwrapped) + } else { + return nil + } + }() + } + } + + public final var nullableString: bridge.std__optional_std__variant_nitro__NullType__std__string__ { + @inline(__always) + get { + return { () -> bridge.std__optional_std__variant_nitro__NullType__std__string__ in + if let __unwrappedValue = self.__implementation.nullableString { + return bridge.create_std__optional_std__variant_nitro__NullType__std__string__({ () -> bridge.std__variant_nitro__NullType__std__string_ in + switch __unwrappedValue { + case .first(let __value): + return bridge.create_std__variant_nitro__NullType__std__string_(margelo.nitro.NullType.null) + case .second(let __value): + return bridge.create_std__variant_nitro__NullType__std__string_(std.string(__value)) + } + }().variant) + } else { + return .init() + } + }() + } + @inline(__always) + set { + self.__implementation.nullableString = { () -> Variant_NullType_String? in + if bridge.has_value_std__optional_std__variant_nitro__NullType__std__string__(newValue) { + let __unwrapped = bridge.get_std__optional_std__variant_nitro__NullType__std__string__(newValue) + return { () -> Variant_NullType_String in + let __variant = bridge.std__variant_nitro__NullType__std__string_(__unwrapped) + switch __variant.index() { + case 0: + let __actual = __variant.get_0() + return .first(NullType.null) + case 1: + let __actual = __variant.get_1() + return .second(String(__actual)) + default: + fatalError("Variant can never have index \(__variant.index())!") + } + }() + } else { + return nil + } + }() + } + } + + public final var optionalCallback: bridge.std__optional_std__function_void____ { + @inline(__always) + get { + return { () -> bridge.std__optional_std__function_void____ in + if let __unwrappedValue = self.__implementation.optionalCallback { + return bridge.create_std__optional_std__function_void____({ () -> bridge.Func_void in + let __closureWrapper = Func_void(__unwrappedValue) + return bridge.create_Func_void(__closureWrapper.toUnsafe()) + }()) + } else { + return .init() + } + }() + } + @inline(__always) + set { + self.__implementation.optionalCallback = { () -> (() -> Void)? in + if bridge.has_value_std__optional_std__function_void____(newValue) { + let __unwrapped = bridge.get_std__optional_std__function_void____(newValue) + return { () -> () -> Void in + let __wrappedFunction = bridge.wrap_Func_void(__unwrapped) + return { () -> Void in + __wrappedFunction.call() + } + }() + } else { + return nil + } + }() + } + } // Methods @inline(__always) diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.cpp b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.cpp index 59067d7f95..3e6768ef11 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.cpp +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.cpp @@ -22,6 +22,12 @@ namespace margelo::nitro::test { prototype.registerHybridSetter("colorScheme", &HybridTestViewSpec::setColorScheme); prototype.registerHybridGetter("someCallback", &HybridTestViewSpec::getSomeCallback); prototype.registerHybridSetter("someCallback", &HybridTestViewSpec::setSomeCallback); + prototype.registerHybridGetter("optionalString", &HybridTestViewSpec::getOptionalString); + prototype.registerHybridSetter("optionalString", &HybridTestViewSpec::setOptionalString); + prototype.registerHybridGetter("nullableString", &HybridTestViewSpec::getNullableString); + prototype.registerHybridSetter("nullableString", &HybridTestViewSpec::setNullableString); + prototype.registerHybridGetter("optionalCallback", &HybridTestViewSpec::getOptionalCallback); + prototype.registerHybridSetter("optionalCallback", &HybridTestViewSpec::setOptionalCallback); prototype.registerHybridMethod("someMethod", &HybridTestViewSpec::someMethod); }); } diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.hpp b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.hpp index 463f363609..e30c8d6794 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.hpp +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/HybridTestViewSpec.hpp @@ -18,6 +18,10 @@ namespace margelo::nitro::test { enum class ColorScheme; } #include "ColorScheme.hpp" #include +#include +#include +#include +#include namespace margelo::nitro::test { @@ -54,6 +58,12 @@ namespace margelo::nitro::test { virtual void setColorScheme(ColorScheme colorScheme) = 0; virtual std::function getSomeCallback() = 0; virtual void setSomeCallback(const std::function& someCallback) = 0; + virtual std::optional getOptionalString() = 0; + virtual void setOptionalString(const std::optional& optionalString) = 0; + virtual std::optional> getNullableString() = 0; + virtual void setNullableString(const std::optional>& nullableString) = 0; + virtual std::optional> getOptionalCallback() = 0; + virtual void setOptionalCallback(const std::optional>& optionalCallback) = 0; public: // Methods diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridRecyclableTestViewComponent.cpp b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridRecyclableTestViewComponent.cpp index 0c838bbd01..14abe27980 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridRecyclableTestViewComponent.cpp +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridRecyclableTestViewComponent.cpp @@ -41,7 +41,7 @@ namespace margelo::nitro::test::views { const react::RawValue* rawValue = rawProps.at("hybridRef", nullptr, nullptr); if (rawValue == nullptr) return sourceProps.hybridRef; const auto& [runtime, value] = (std::pair)*rawValue; - return CachedProp& /* ref */)>>>::fromRawValue(*runtime, value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.hybridRef); + return CachedProp& /* ref */)>>>::fromRawValue(*runtime, (value.isNull() || value.isUndefined()) ? jsi::Value::undefined() : value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.hybridRef); } catch (const std::exception& exc) { throw std::runtime_error(std::string("RecyclableTestView.hybridRef: ") + exc.what()); } diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.cpp b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.cpp index 3576364517..c9d8d5d1d7 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.cpp +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.cpp @@ -61,17 +61,47 @@ namespace margelo::nitro::test::views { const react::RawValue* rawValue = rawProps.at("someCallback", nullptr, nullptr); if (rawValue == nullptr) return sourceProps.someCallback; const auto& [runtime, value] = (std::pair)*rawValue; - return CachedProp>::fromRawValue(*runtime, value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.someCallback); + return CachedProp>::fromRawValue(*runtime, (value.isNull() || value.isUndefined()) ? jsi::Value::undefined() : value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.someCallback); } catch (const std::exception& exc) { throw std::runtime_error(std::string("TestView.someCallback: ") + exc.what()); } }()), + optionalString([&]() -> CachedProp> { + try { + const react::RawValue* rawValue = rawProps.at("optionalString", nullptr, nullptr); + if (rawValue == nullptr) return sourceProps.optionalString; + const auto& [runtime, value] = (std::pair)*rawValue; + return CachedProp>::fromRawValue(*runtime, value, sourceProps.optionalString); + } catch (const std::exception& exc) { + throw std::runtime_error(std::string("TestView.optionalString: ") + exc.what()); + } + }()), + nullableString([&]() -> CachedProp>> { + try { + const react::RawValue* rawValue = rawProps.at("nullableString", nullptr, nullptr); + if (rawValue == nullptr) return sourceProps.nullableString; + const auto& [runtime, value] = (std::pair)*rawValue; + return CachedProp>>::fromRawValue(*runtime, value, sourceProps.nullableString); + } catch (const std::exception& exc) { + throw std::runtime_error(std::string("TestView.nullableString: ") + exc.what()); + } + }()), + optionalCallback([&]() -> CachedProp>> { + try { + const react::RawValue* rawValue = rawProps.at("optionalCallback", nullptr, nullptr); + if (rawValue == nullptr) return sourceProps.optionalCallback; + const auto& [runtime, value] = (std::pair)*rawValue; + return CachedProp>>::fromRawValue(*runtime, (value.isNull() || value.isUndefined()) ? jsi::Value::undefined() : value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.optionalCallback); + } catch (const std::exception& exc) { + throw std::runtime_error(std::string("TestView.optionalCallback: ") + exc.what()); + } + }()), hybridRef([&]() -> CachedProp& /* ref */)>>> { try { const react::RawValue* rawValue = rawProps.at("hybridRef", nullptr, nullptr); if (rawValue == nullptr) return sourceProps.hybridRef; const auto& [runtime, value] = (std::pair)*rawValue; - return CachedProp& /* ref */)>>>::fromRawValue(*runtime, value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.hybridRef); + return CachedProp& /* ref */)>>>::fromRawValue(*runtime, (value.isNull() || value.isUndefined()) ? jsi::Value::undefined() : value.asObject(*runtime).getProperty(*runtime, PropNameIDCache::get(*runtime, "f")), sourceProps.hybridRef); } catch (const std::exception& exc) { throw std::runtime_error(std::string("TestView.hybridRef: ") + exc.what()); } @@ -83,6 +113,9 @@ namespace margelo::nitro::test::views { case hashString("hasBeenCalled"): return true; case hashString("colorScheme"): return true; case hashString("someCallback"): return true; + case hashString("optionalString"): return true; + case hashString("nullableString"): return true; + case hashString("optionalCallback"): return true; case hashString("hybridRef"): return true; default: return false; } diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.hpp b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.hpp index 2e2f681b1d..aed50870d8 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.hpp +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/c++/views/HybridTestViewComponent.hpp @@ -18,9 +18,12 @@ #include "ColorScheme.hpp" #include +#include +#include +#include +#include #include #include "HybridTestViewSpec.hpp" -#include namespace margelo::nitro::test::views { @@ -46,6 +49,9 @@ namespace margelo::nitro::test::views { CachedProp hasBeenCalled; CachedProp colorScheme; CachedProp> someCallback; + CachedProp> optionalString; + CachedProp>> nullableString; + CachedProp>> optionalCallback; CachedProp& /* ref */)>>> hybridRef; private: diff --git a/packages/react-native-nitro-test/nitrogen/generated/shared/json/TestViewConfig.json b/packages/react-native-nitro-test/nitrogen/generated/shared/json/TestViewConfig.json index ef9bbaaf6c..02938ffe23 100644 --- a/packages/react-native-nitro-test/nitrogen/generated/shared/json/TestViewConfig.json +++ b/packages/react-native-nitro-test/nitrogen/generated/shared/json/TestViewConfig.json @@ -8,6 +8,9 @@ "hasBeenCalled": true, "colorScheme": true, "someCallback": true, + "optionalString": true, + "nullableString": true, + "optionalCallback": true, "hybridRef": true } } diff --git a/packages/react-native-nitro-test/src/specs/TestView.nitro.ts b/packages/react-native-nitro-test/src/specs/TestView.nitro.ts index 8031c5e585..f6851bf1bc 100644 --- a/packages/react-native-nitro-test/src/specs/TestView.nitro.ts +++ b/packages/react-native-nitro-test/src/specs/TestView.nitro.ts @@ -11,6 +11,9 @@ export interface TestViewProps extends HybridViewProps { hasBeenCalled: boolean colorScheme: ColorScheme someCallback: () => void + optionalString?: string + nullableString?: string | null + optionalCallback?: () => void } export interface TestViewMethods extends HybridViewMethods { someMethod(): void From 3bde52e52a597c17466aa8e629caa34cab540d4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikl=C3=B3s=20Fazekas?= Date: Wed, 8 Jul 2026 15:51:46 +0200 Subject: [PATCH 2/2] fix: Keep original error for required props, pin `T | null` clearing semantics --- example/__tests__/views.harness.tsx | 55 ++++++++++++------- .../cpp/views/CachedProp.hpp | 7 ++- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/example/__tests__/views.harness.tsx b/example/__tests__/views.harness.tsx index f1be5ac1b6..46f8684ed0 100644 --- a/example/__tests__/views.harness.tsx +++ b/example/__tests__/views.harness.tsx @@ -95,6 +95,20 @@ describe('TestView optional props', () => { } } + // 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 { + 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() @@ -109,9 +123,7 @@ describe('TestView optional props', () => { await waitFor(() => expect(view.optionalString).toBe('hello')) update({}) - await waitFor(() => - expect(errors.length > 0 || view.optionalString === undefined).toBe(true) - ) + await waitForUpdateOrError(errors, () => view.optionalString === undefined) expectNoRenderErrors(errors) expect(view.optionalString).toBeUndefined() expect(mountCount()).toBe(1) @@ -124,9 +136,7 @@ describe('TestView optional props', () => { await waitFor(() => expect(view.optionalString).toBe('hello')) update({ optionalString: undefined }) - await waitFor(() => - expect(errors.length > 0 || view.optionalString === undefined).toBe(true) - ) + await waitForUpdateOrError(errors, () => view.optionalString === undefined) expectNoRenderErrors(errors) expect(view.optionalString).toBeUndefined() expect(mountCount()).toBe(1) @@ -139,13 +149,9 @@ describe('TestView optional props', () => { await waitFor(() => expect(view.optionalString).toBe('first')) update({}) - await waitFor(() => - expect(errors.length > 0 || view.optionalString === undefined).toBe(true) - ) + await waitForUpdateOrError(errors, () => view.optionalString === undefined) update({ optionalString: 'second' }) - await waitFor(() => - expect(errors.length > 0 || view.optionalString === 'second').toBe(true) - ) + await waitForUpdateOrError(errors, () => view.optionalString === 'second') expectNoRenderErrors(errors) expect(view.optionalString).toBe('second') }) @@ -157,10 +163,9 @@ describe('TestView optional props', () => { await waitFor(() => expect(view.optionalCallback).toBeDefined()) update({}) - await waitFor(() => - expect(errors.length > 0 || view.optionalCallback === undefined).toBe( - true - ) + await waitForUpdateOrError( + errors, + () => view.optionalCallback === undefined ) expectNoRenderErrors(errors) expect(view.optionalCallback).toBeUndefined() @@ -176,9 +181,21 @@ describe('TestView optional props', () => { // `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 waitFor(() => - expect(errors.length > 0 || view.nullableString === null).toBe(true) - ) + 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() }) diff --git a/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp b/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp index 839b023c98..7619ec7ac2 100644 --- a/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp +++ b/packages/react-native-nitro-modules/cpp/views/CachedProp.hpp @@ -62,7 +62,12 @@ struct CachedProp { // 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 - return JSIConverter::fromJSI(runtime, jsi::Value::undefined()); + jsi::Value undefined = jsi::Value::undefined(); + if (JSIConverter::canConvert(runtime, undefined)) { + return JSIConverter::fromJSI(runtime, undefined); + } + // T can't represent "no value" either (required prop) - fall through so + // the error reports the actual (null) value. } return JSIConverter::fromJSI(runtime, value); }