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

[zhongwuzw]

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]

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

[cipolleschi]

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]

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]

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

[javache]

Should this be looking at convertedObjCArg instead?

[zhongwuzw]

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 .

[javache]

@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.

[zhongwuzw]

Oops, you're right. 👍 updated.

[cipolleschi]

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:

1. a unit test
2. 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]

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

[cipolleschi]

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]

@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.

[cipolleschi]

Why are we passing enableModuleArgumentNSNullConversionIOS to the convertJSIValueToObjCObject function?

[zhongwuzw]

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]

~~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]

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.

[javache]

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