fix: Treat null as undefined for removed optional View props - #1515
Open
mrousavy wants to merge 1 commit into
Open
fix: Treat null as undefined for removed optional View props#1515mrousavy wants to merge 1 commit into
null as undefined for removed optional View props#1515mrousavy wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
React Fabric diff turns a removed prop into an explicit null sentinel (and prop={undefined} takes the same path). Nitro ReactProp passed that null straight to JSIConverter, which throws for non-null types, so removing any previously-provided optional View prop (including hybridRef, which crashed earlier in the { f } unwrap) failed the Fabric commit.
Now, if the prop type is optional and null is not a valid value for it (checked via JSIConverter<T>::canConvert), the value is normalized to undefined and the setter is called once with nullopt. Props that can legitimately hold null (NullType, variants containing null) are untouched, and required props still throw loudly. Zero overhead for non-optional props (the branch compiles out), one isNull() tag check for optional ones.
mrousavy
force-pushed
the
fix/view-optional-prop-removal
branch
from
August 21, 2026 10:11
737d1c7 to
089bc77
Compare
mfazekas
added a commit
to rive-app/rive-nitro-react-native
that referenced
this pull request
Aug 23, 2026
Generated updateProps now diffs the new Props snapshot against the old one instead of consuming isDirty flags that live on the shared Props object (margelo/nitro#1503, #1506, #1510), so a component view Fabric recreates from an unchanged ShadowNode gets every provided prop applied again rather than nothing. That is what view-recreate.harness.tsx was written for. The peer range moves with it, so consumers have to bump too. Prop parsing also moved into Nitro core, which leaves the post-process shim for margelo/nitro#1184 without a parse site to patch — but that bug is still live, so clearing an optional prop throws mid-commit ("RiveView. layoutScaleFactor: Value is null, expected a number"). Carry mrousavy's own fix, margelo/nitro#1515, as a yarn patch until it ships, and cover both the optional<double> and the variant shape with a harness test. The generated EventPropertiesOutput variant is now ordered `boolean | number | string`, so the two hand-written helpers swap .second and .third. Both example apps make glog non-modular in post_install: 0.37.0 exposes React's renderer headers in the NitroModules modulemap and they reach <glog/logging.h>, whose headers include from inside `namespace google` — illegal once glog is a module, so every target that builds the NitroModules module fails to compile. Nothing in React Native imports glog as a module.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
undefinedto optional view property #1184Summary
nullsentinel for a removed prop (ReactNativeAttributePayload: "Flag the leaf property for removal by sending a sentinel") - andprop={undefined}takes the same path, so the two are indistinguishable on the wireCachedProppreviously passed thatnullstraight into conversion, which throws mid-commit for every optional prop type that rejects null (optional<double>viaasNumber(), andhybridRefeven earlier, in the{ f }function-prop unwrap)nullis not a valid value for it (JSIConverter<T>::canConvert), the value is normalized toundefined→ the native setter is called once withnulloptSemantics
undefined→ setter receivesnullopt(the JS-visible value becomesundefined). The native initializer default still only covers never-provided props - a richer "reset on clear" belongs in the implementation (storage ?? default)null(NullType, variants containing null): untouched -nullstays data, so removal collapses to "explicit null" for those (inherent to React's sentinel protocol)ViewName.propName:prefixBecause normalization only applies where conversion was guaranteed to throw, no currently-working behavior can change.
Overhead
if constexpr (is_optional<T>)branch compiles outvalue.isNull()tag check during Props parsing; thecanConvertwalk only runs when the value actually isnull(i.e. an actual removal)Testing
New Harness test
delivers undefined when an optional prop is removed": mounts TestView withnativeDefaultValue={1}+hybridRef, then removes both - asserts the value readsundefined, the setter fired exactly once more, unrelated setters (isBlue) did not fire, and the previously-crashinghybridRef` removal path survives. Red without the fix (commit-time throw), green with it.Validated: workspace typecheck, eslint (CI settings), clang-format.
🤖 Generated with Claude Code