fix: Fix clearing optional view props crashing with "Value is null"#1432
Open
mfazekas wants to merge 2 commits into
Open
fix: Fix clearing optional view props crashing with "Value is null"#1432mfazekas wants to merge 2 commits into
mfazekas wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
6 tasks
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
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.
Fixes #1184. Alternative to #1189: instead of loosening
JSIConverter<std::optional<T>>for all conversions, this mapsnullto "no value" only for view props -CachedProp::fromRawValuetreatsnulllikeundefined, unless the prop's type explicitly models null (e.g.string | null). Method arguments and hybrid object properties stay strict aboutnullvsundefined.For view props that distinction doesn't exist anyway: React sends
nullboth for a removed prop (ReactNativeAttributePayload.js#L390-L394) and for an explicitundefined(#L270-L277), so the native side cannot tell them apart - it just crashed the commit withException in HostFunction: <View>.<prop>: Value is null, expected a String.Also covers optional callback props (not fixed by #1189): those crashed earlier, in the generated
{ f: ... }unwrap (value.asObject()), before any converter ran.Notes:
foo?: string | null), clearing the prop deliversnull, notundefined- React's payload makes the two indistinguishable, and null-modeling types takenullat face value. Pinned by a test.nullpayload still throws, with the original "Value is null" message.CachedProppart ships withreact-native-nitro-modulesdirectly.Tests: optional props on the existing
TestView+ on-device harness tests inexample/__tests__/views.harness.tsx(clearing a prop needs React's commit path, so this can't live ingetTests.ts). They fail onmain(#1433) and pass here, verified on iOS and Android.Checklist
main, narrow scope.main(test: Add failing tests for clearing optional view props (#1184) #1433), passes with the fix.bun specsrun, generated files committed.bun lint-allpasses locally.