[iOS] Turbo module: Fixes dictionary stripped out when value is null #51103

zhongwuzw · 2025-05-05T14:45:33Z

Summary:

Fixes #51083. Turbo stripped out the dictionary when the value is null. The old architecture transforms null to NSNull. The null seems useful in cases like #51803 for removing the storage of the key. @cipolleschi can you please help to review?

Changelog:

[IOS] [FIXED] - Turbo module: Fixes dictionary stripped out when value is null

Test Plan:

Repro please see #51083.

facebook-github-bot · 2025-05-05T22:19:10Z

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2025-05-06T09:05:03Z

Hi @zhongwuzw! thanks for the PR.
I'm not sure we can land this. I know that this is a semantic change between the Old and the New Architecture. However this might be a deliberate design decision, made before me joining the team. I'll have to investigate.

Internally, we have several pieces of code that relies on the null values being removed from the dictionary, so by landing this, we are going to break that code. Similarly, we might be breaking New Arch libraries that do not expect receiving NSNull.

zhongwuzw · 2025-05-06T16:06:29Z

Hi @zhongwuzw! thanks for the PR.

I'm not sure we can land this. I know that this is a semantic change between the Old and the New Architecture. However this might be a deliberate design decision, made before me joining the team. I'll have to investigate.

Internally, we have several pieces of code that relies on the null values being removed from the dictionary, so by landing this, we are going to break that code. Similarly, we might be breaking New Arch libraries that do not expect receiving NSNull.

@cipolleschi Thanks for the review. If we don't support null vaule of dictionary, we should seems make the native module like setting module add a removeKey api to not passed dictionary?

javache · 2025-05-06T16:35:00Z

Thanks for fixing this! I think this absolutely needs a unit test to show how the behaviour changes before/after.

javache · 2025-05-07T11:21:41Z

.../react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm

@@ -639,6 +639,10 @@ TraceSection s(
          // Message dispatch logic from old infra
          id (*convert)(id, SEL, id) = (__typeof__(convert))objc_msgSend;
          id convertedObjCArg = convert([RCTConvert class], rctConvertSelector, objCArg);
+
+          if (objCArg == [NSNull null]) {


Should this be looking at convertedObjCArg instead?

Actually I keep the same logic as RCTInteropTurboModule : ) https://github.com/facebook/react-native/blob/main/packages/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTInteropTurboModule.mm#L475-L483 .

@zhongwuzw: that doesn't seem right. RCTInteropTurboModule looks at arg, which is the return value of the RCTConvertTo call (we should probably use that RCTConvertTo helper here too!) - so the equivalent would be to use convertedObjCArg here.

Oops, you're right. 👍 updated.

cipolleschi · 2025-05-08T09:30:31Z

As @javache was mentioning, we discussed this internally and we want to pull in the fix.
However, this change might break quite a few stuff, so we need to be cautious about how we do it.
for sure we will need:

a unit test
a feature flag, that we can use to revert to the previous behavior in case too many things start to go very bad. We have a good infrastructure for feature flags: you can modify the featureflag.config.js to add it. Then yarn featureflags --update should generate all the required files. Then you should import the feature flag and use it.

Do you mind applying these changes?

zhongwuzw · 2025-05-08T09:32:56Z

@cipolleschi Got it. I'll try to do them.

cipolleschi · 2025-05-09T16:34:09Z

I tried to apply this fix to a test that I was doing for #51083, but it does not fixes.
I was using this repo as test bed: https://github.com/cipolleschi/TMDroppingNull

It contains both a legacy native module and a turbomodule.

The expectation is that both should receive

{
  "key": "myKey",
  "value": "<null>"
}

when pressing the Save Null button in the UI

zhongwuzw · 2025-05-11T02:30:55Z

@cipolleschi Hi, I tried, seems I received the null value.
LegacyNativeModule:

turbomodule;

BTW, can you help me how to write a test? I don't know wether unit test needs some env requirements? Seems I only see the old bridge related unit test. I used RNTester RNTesterUnitTests schema.

This reverts commit f09fced.

…/fix_dictionary_not_passed_nil_value

cipolleschi · 2025-05-14T15:51:15Z

.../react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm

-  id objCArg = convertJSIValueToObjCObject(runtime, arg, jsInvoker_);
+  BOOL enableModuleArgumentNSNullConversionIOS = ReactNativeFeatureFlags::enableModuleArgumentNSNullConversionIOS();
+  id objCArg = TurboModuleConvertUtils::convertJSIValueToObjCObject(
+      runtime, arg, jsInvoker_, enableModuleArgumentNSNullConversionIOS);


Why are we passing enableModuleArgumentNSNullConversionIOS to the convertJSIValueToObjCObject function?

We enable useNull only if we set enableModuleArgumentNSNullConversionIOS to true to convert null to NSNull. If we don't enable enableModuleArgumentNSNullConversionIOS, everything remains the same as original.

cipolleschi · 2025-05-14T16:02:09Z

~~I tried again with the repo I prepared and I don't get the same result as you. 😰 ~~

My bad, I realized that I applied badly your suggestion.

For the unit test, I'm not sure we have something set up in OSS. I'll have to investigate a bit and get back to you.

javache · 2025-05-15T15:51:32Z

I'm adding a test in #51370 - which we should be able to use verify this changes fixes it once you enable the feature flag.

…/fix_dictionary_not_passed_nil_value

javache · 2025-05-20T10:20:18Z

Can you update the test and add a variant that shows the behaviour you get when overriding the feature flag?

zhongwuzw · 2025-05-21T08:25:07Z

@javache Hi, can you tell me how to run the test? I already merged your commit

react-native/packages/react-native/ReactCommon/react/nativemodule/core/iostests/RCTTurboModuleTests.mm

Line 89 in 6d25274

OCMVerify(OCMTimes(1), [instance_ testMethodWhichTakesObject:@{}]);

and I see it already added dictionary null value check, I don't know why my pr's ci is all passed, my featureflag is enabled by default.

facebook-github-bot · 2025-05-21T10:03:28Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2025-05-21T10:04:07Z

Ah, we probably don't have these tests setup for OSS CI :( I'll run it internally

facebook-github-bot · 2025-05-21T10:23:34Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

javache · 2025-05-21T10:32:30Z

Could you try updating the PR? I'm having trouble importing it.

…/fix_dictionary_not_passed_nil_value

zhongwuzw · 2025-05-21T10:36:56Z

@javache Hi, I updated .

facebook-github-bot · 2025-05-21T11:27:59Z

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

…ook#51103) Summary: Fixes facebook#51083. Turbo stripped out the dictionary when the value is null. The old architecture transforms null to NSNull. The null seems useful in cases like facebook#51803 for removing the storage of the key. cipolleschi can you please help to review? ## Changelog: [IOS] [FIXED] - Turbo module: Fixes dictionary stripped out when value is null Pull Request resolved: facebook#51103 Test Plan: Repro please see facebook#51083. Differential Revision: D74208525 Pulled By: javache

react-native-bot · 2025-05-23T13:43:21Z

This pull request was successfully merged by @zhongwuzw in 4a4fd1c

^{When will my fix make it into a release? | How to file a pick request?}

facebook-github-bot · 2025-05-23T13:43:22Z

@javache merged this pull request in 4a4fd1c.

Summary: This is causing some internal test failures for now, so disabling the behaviour to prevent further rollout. Changelog: [iOS] Disable fix for facebook#51103 until more testing can be done. Differential Revision: D75320842

Summary: Pull Request resolved: #51576 This is causing some internal test failures for now, so disabling the behaviour to prevent further rollout. Changelog: [iOS][Removed] Disable fix for #51103 until more testing can be done. Differential Revision: D75320842 fbshipit-source-id: 39c115afd11e5b1aca6cdc1fc18ec7e83eb10382

javache · 2025-05-27T16:24:39Z

This change caused some internal issues, so I disabled it by default again, I'll try to debug more but this may affect other new architecture users too.

[iOS] Turbo module: Fixes dictionary stripped out when value is null

4f46cd6

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels May 5, 2025

javache reviewed May 7, 2025

View reviewed changes

zhongwuzw added 8 commits May 11, 2025 10:59

Add featureflags

f09fced

Revert "Add featureflags"

d8c6378

This reverts commit f09fced.

Merge commit '996be870713cd72df1f91db99e8f981bbc5406af' into features…

1ac1c7d

…/fix_dictionary_not_passed_nil_value

update featureflag

3931ee9

format

4c39c75

Merge branch 'main' into features/fix_dictionary_not_passed_nil_value

4cfb407

update feature flags signedSource

3efbf25

Fixes wrong parameter

e5b79ff

cipolleschi reviewed May 14, 2025

View reviewed changes

zhongwuzw added 2 commits May 19, 2025 18:54

Merge commit '8d8cb718aa703b79cc1b1b639e54fb52b12a3b25' into features…

36ed074

…/fix_dictionary_not_passed_nil_value

update featureflag

1aa89bd

zhongwuzw added 2 commits May 21, 2025 18:34

Merge commit 'bcfa5ca474a803f86ac3073f133bc5a83c6d755c' into features…

2632dcf

…/fix_dictionary_not_passed_nil_value

update

59451d2

facebook-github-bot closed this in 4a4fd1c May 23, 2025

react-native-bot added the Merged This PR has been merged. label May 23, 2025

javache mentioned this pull request May 23, 2025

Disable enableModuleArgumentNSNullConversionIOS by default #51576

Closed

[iOS] Turbo module: Fixes dictionary stripped out when value is null #51103

[iOS] Turbo module: Fixes dictionary stripped out when value is null #51103

Uh oh!

Conversation

zhongwuzw commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Changelog:

Test Plan:

Uh oh!

facebook-github-bot commented May 5, 2025

Uh oh!

cipolleschi commented May 6, 2025

Uh oh!

zhongwuzw commented May 6, 2025

Uh oh!

javache commented May 6, 2025

Uh oh!

javache May 7, 2025

Choose a reason for hiding this comment

Uh oh!

zhongwuzw May 7, 2025

Choose a reason for hiding this comment

Uh oh!

javache May 12, 2025

Choose a reason for hiding this comment

Uh oh!

zhongwuzw May 12, 2025

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented May 8, 2025

Uh oh!

zhongwuzw commented May 8, 2025

Uh oh!

cipolleschi commented May 9, 2025

Uh oh!

zhongwuzw commented May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cipolleschi May 14, 2025

Choose a reason for hiding this comment

Uh oh!

zhongwuzw May 15, 2025

Choose a reason for hiding this comment

Uh oh!

cipolleschi commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javache commented May 15, 2025

Uh oh!

javache commented May 20, 2025

Uh oh!

zhongwuzw commented May 21, 2025

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

javache commented May 21, 2025

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

javache commented May 21, 2025

Uh oh!

zhongwuzw commented May 21, 2025

Uh oh!

facebook-github-bot commented May 21, 2025

Uh oh!

react-native-bot commented May 23, 2025

Uh oh!

facebook-github-bot commented May 23, 2025

Uh oh!

javache commented May 27, 2025

Uh oh!

Uh oh!

zhongwuzw commented May 5, 2025 •

edited

Loading

zhongwuzw commented May 11, 2025 •

edited

Loading

cipolleschi commented May 14, 2025 •

edited

Loading