react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Fabric component: codegen drops optional state for component properties and in events properties

Open wvanhaevre opened this issue 10 months ago • 21 comments

Description

When adding optional values to the specs for a Fabric component, these optionals are always bridged with a default value and as a result are always defined and no longer optional.

Example spec:

type Prop1 = Readonly<{
    propContent: string;
    propOptionalContent1?: string;
    propOptionalContent2?: string;
}>;
type Prop2 = Readonly<{
    propContent: string;
    propOptionalContent1?: string;
    propOptionalContent2?: string;
}>;
export type NativeEvent1 = Readonly<{
    nativeEventContent: string;
    nativeEventOptionalContent1?: string;
    nativeEventOptionalContent2?: string;
}>;
export type NativeEvent2 = Readonly<{
    nativeEventContent: string;
    nativeEventOptionalContent1?: string;
    nativeEventOptionalContent2?: string;
}>;
export interface NewArchViewProps extends ViewProps {
    prop1: Prop1;
    prop2: Prop2;
    onNativeEvent1: DirectEventHandler<NativeEvent1>;
    onNativeEvent2: DirectEventHandler<NativeEvent2>;
}

Codegen creates:

struct NewArchViewProp1Struct {
  std::string propContent{};
  std::string propOptionalContent1{};
  std::string propOptionalContent2{};
};
struct NewArchViewProp2Struct {
  std::string propContent{};
  std::string propOptionalContent1{};
  std::string propOptionalContent2{};
};
struct OnNativeEvent1 {
  std::string nativeEventContent;
  std::string nativeEventOptionalContent1;
  std::string nativeEventOptionalContent2;
};
struct OnNativeEvent2 {
  std::string nativeEventContent;
  std::string nativeEventOptionalContent1;
  std::string nativeEventOptionalContent2;
};

Every time an optional value is left out on 1 side of the bridge, the other side receives a default value (e.g optional string is filled in with empty string ""). Setting a default using WithDefault<> gives control over this default, but still does not respect the optional status of the properties.

As a result the old architecture versus the new architecture result in completely different objects being bridged. One has optional data where the other is always prop complete but with defaults.

Steps to reproduce

Add the above specs to a new arch enabled project and check the generated props.h and EventEmmiter.h for the generated code.

The linked reproducer has these in place.

React Native Version

0.76.7

Affected Platforms

Runtime - iOS

Areas

Codegen

Output of npx @react-native-community/cli info

System:
  OS: macOS 15.0.1
  CPU: (8) arm64 Apple M1 Pro
  Memory: 298.75 MB / 32.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 22.11.0
    path: ~/.nvm/versions/node/v22.11.0/bin/node
  Yarn:
    version: 1.22.22
    path: ~/.nvm/versions/node/v22.11.0/bin/yarn
  npm:
    version: 10.9.0
    path: ~/.nvm/versions/node/v22.11.0/bin/npm
  Watchman:
    version: 2024.08.19.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.16.2
    path: /opt/homebrew/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 24.2
      - iOS 18.2
      - macOS 15.2
      - tvOS 18.2
      - visionOS 2.2
      - watchOS 11.2
  Android SDK: Not Found
IDEs:
  Android Studio: 2024.1 AI-241.18034.62.2411.12169540
  Xcode:
    version: 16.2/16C5032a
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.14
    path: /Library/Java/JavaVirtualMachines/zulu-17.jdk/Contents/Home/bin/javac
  Ruby:
    version: 3.3.5
    path: /opt/homebrew/opt/ruby/bin/ruby
npmPackages:
  "@react-native-community/cli":
    installed: 15.0.1
    wanted: 15.0.1
  react:
    installed: 18.3.1
    wanted: 18.3.1
  react-native:
    installed: 0.76.5
    wanted: 0.76.5
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: true
iOS:
  hermesEnabled: true
  newArchEnabled: true

Stacktrace or Logs

Example logs from the reproducer (reproducer/no-optionals branch) showing empty strings being bridged instead of remaining optional:

'nativeEvent1 optionalContent1 received:', '76BF877F-B476-4DB6-B961-C0DDA76B91E1'
'nativeEvent1 optionalContent2 received:', '24E3266D-7451-40E4-8A38-B5931A9ACDD1'
'nativeEvent2 optionalContent1 received:', ''
'nativeEvent2 optionalContent2 received:', ''
propOptionalContent1: optional1
propOptionalContent2: optional2
propOptionalContent1:
propOptionalContent2:

Reproducer

https://github.com/wvanhaevre/NewArchReproducer/tree/reproducer/no-optionals

Screenshots and Videos

No response

wvanhaevre avatar Mar 10 '25 10:03 wvanhaevre

[!TIP] Newer version available: You are on a supported minor version, but it looks like there's a newer patch available - 0.76.7. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

react-native-bot avatar Mar 10 '25 10:03 react-native-bot

[!TIP] Newer version available: You are on a supported minor version, but it looks like there's a newer patch available - undefined. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.

react-native-bot avatar Mar 10 '25 10:03 react-native-bot

Checked with 0.76.7 and issue still remains.

wvanhaevre avatar Mar 10 '25 11:03 wvanhaevre

@wvanhaevre Thanks for opening the issue. We will look into it as soon as we can.

cipolleschi avatar Mar 10 '25 12:03 cipolleschi

Codegen definitely has support for optional props:

https://github.com/facebook/react-native/blob/c04b4a92d1161c46afe855268bce0366c71b68fa/packages/react-native-codegen/src/generators/modules/GenerateModuleH.js#L141-L228

See the various wrapOptional here that should wrap the types with an std::optional. We'll have to debug a bit to understand why is it not working as expected in your scenario

cortinico avatar Mar 10 '25 16:03 cortinico

@cortinico , just to make sure, adding the ? To the spec description is enough to make the property optional, right? i.e.

type Prop1 = Readonly<{
    propContent: string;
    propOptionalContent1?: string;
    propOptionalContent2?: string;
}>;

makes the 2 last string optional and should appear on the C++ side as std::optional<std::string> ?

wvanhaevre avatar Mar 10 '25 16:03 wvanhaevre

I think that there is a bit of confusion:

this is an optional property:

type Prop1 = Readonly<{
    propContent: string;
    propOptionalContent1: string | null; 
    propOptionalContent2: string | null;
}>;

and it will result in

struct NewArchViewProp1Struct {
  std::string propContent{};
  std::optional<std::string> propOptionalContent1{};
  std::optional<std::string> propOptionalContent2{};
};

The prop will always be there, but propOptionalContent1{} can be either null or can have a proper value like "".

The actual problem is that a struct in C++ can't have "undefined" properties.

In JS:

let a = { a: 10 }
a.b // undefined

Is valid code and a.b returns undefined.

In C++, you can't try to access a field that doesn't exists. The app will not compile at all.

I think that the reason why

type Prop1 = Readonly<{
    propContent: string;
    propOptionalContent1?: string;
    propOptionalContent2?: string;
}>;

is mapped to a std::string with a default value is because in C++ you can't define a struct with a property that can be missing.

The way to do it would be to generate something like:

struct NewArchViewProp1AStruct {
  std::string propContent{};
  std::string propOptionalContent1{};
  std::string propOptionalContent2{};
};

struct NewArchViewProp1BStruct {
  std::string propContent{};
  std::string propOptionalContent1{};
};

struct NewArchViewProp1CStruct {
  std::string propContent{};
  std::string propOptionalContent2{};
};

struct NewArchViewProp1DStruct {
  std::string propContent{};
};

type Prop1 = std::variant<NewArchViewProp1AStruct, NewArchViewProp1BStruct, NewArchViewProp1CStruct, NewArchViewProp1DStruct>

but, as you can see, this explodes combinatorially with the number of potentially undefined properties.

Bottom line is that: yes, there is a difference between the old arch and the new arch. This difference should be addressed as part of the migration. I don't think we can actually fix this in a type-safe way.

cipolleschi avatar Mar 10 '25 16:03 cipolleschi

We are indeed looking for a way to get this generated:

struct NewArchViewProp1Struct {
  std::string propContent{};
  std::optional<std::string> propOptionalContent1{};
  std::optional<std::string> propOptionalContent2{};
};

where not setting the std::optional properties results in them being null. We would expect (in case of an event property) that the object that is received on the JS side does not contain the propOptionalContent1 and propOptionalContent2 when set to null on the C++ side.

This is however not working out.

When we define in the spec:

type Prop1 = Readonly<{
    propContent: string;
    propOptionalContent1: string | null; 
    propOptionalContent2: string | null;
}>;

We still get as generated struct:

struct NewArchViewProp1Struct {
  std::string propContent{};
  std::string propOptionalContent1{};
  std::string propOptionalContent2{};
};

wvanhaevre avatar Mar 10 '25 17:03 wvanhaevre

@wvanhaevre are you using typescript to define your interfaces? I tried to reproduce the issue and, effectively, the optional part is never generated:

Specs:

import type {HostComponent, ViewProps} from 'react-native';
import type {BubblingEventHandler} from 'react-native/Libraries/Types/CodegenTypes';
import codegenNativeComponent from 'react-native/Libraries/Utilities/codegenNativeComponent';

type WebViewScriptLoadedEvent = {
  result: 'success' | 'error';
};

type MyType = {
  foo: string;
  bar?: string;
  baz: string | undefined | null;
  barbaz?: string | undefined | null;
}

export interface NativeProps extends ViewProps {
  sourceURL?: string;
  onScriptLoaded?: BubblingEventHandler<WebViewScriptLoadedEvent> | null;
  myType: MyType;

  bar?: string;
  baz: string | undefined | null;
  barbaz?: string | undefined | null;
}

export default codegenNativeComponent<NativeProps>(
  'CustomWebView',
) as HostComponent<NativeProps>;

Generated code:

#pragma once

#include <react/renderer/components/view/ViewProps.h>
#include <react/renderer/core/PropsParserContext.h>
#include <react/renderer/core/propsConversions.h>

namespace facebook::react {

struct CustomWebViewMyTypeStruct {
  std::string foo{};
  std::string bar{};
  std::string baz{};
  std::string barbaz{};
};

static inline void fromRawValue(const PropsParserContext& context, const RawValue &value, CustomWebViewMyTypeStruct &result) {
  auto map = (std::unordered_map<std::string, RawValue>)value;

  auto tmp_foo = map.find("foo");
  if (tmp_foo != map.end()) {
    fromRawValue(context, tmp_foo->second, result.foo);
  }
  auto tmp_bar = map.find("bar");
  if (tmp_bar != map.end()) {
    fromRawValue(context, tmp_bar->second, result.bar);
  }
  auto tmp_baz = map.find("baz");
  if (tmp_baz != map.end()) {
    fromRawValue(context, tmp_baz->second, result.baz);
  }
  auto tmp_barbaz = map.find("barbaz");
  if (tmp_barbaz != map.end()) {
    fromRawValue(context, tmp_barbaz->second, result.barbaz);
  }
}

static inline std::string toString(const CustomWebViewMyTypeStruct &value) {
  return "[Object CustomWebViewMyTypeStruct]";
}
class CustomWebViewProps final : public ViewProps {
 public:
  CustomWebViewProps() = default;
  CustomWebViewProps(const PropsParserContext& context, const CustomWebViewProps &sourceProps, const RawProps &rawProps);

#pragma mark - Props

  std::string sourceURL{};
  CustomWebViewMyTypeStruct myType{};
  std::string bar{};
  std::string baz{};
  std::string barbaz{};
};

} // namespace facebook::react

We need to investigate further.

The behavior is consistent between TS and Flow.

cipolleschi avatar Mar 10 '25 17:03 cipolleschi

Yep... components does not support std::optional currently. The file responsible to generate the types for the components is ComponentsGeneratorUtils.js. If you search for std::optional, there are no occurrences of it.

@cortinico the file you were looking at is in the modules folder, and it is only used for Native Modules. They indeed supports optional values.

cipolleschi avatar Mar 10 '25 17:03 cipolleschi

Thanks for evaluating.

Is there an expectation on when this could be added for Fabric Components? I'm working on a project that has quiet complex objects with lots of optional data. On the old architecture these fly across the bridge perfectly, but using the new architecture we end up with objects containing a lot of unwanted default values. It's often hard to distinguish between an automatically assigned default value (even a custom one) or an actually defined value.

wvanhaevre avatar Mar 10 '25 17:03 wvanhaevre

Is there an expectation on when this could be added for Fabric Components?

We don't have an ETA at the moment. The fastest way to get this in is to submit a Pull Request and get either @cipolleschi or me to help you merging it.

cortinico avatar Mar 11 '25 09:03 cortinico

@cortinico Thanks for the feedback!

Currently we are using the interop layer and there all of these features do seem supported - we're struggling in particular with these optional properties and with the fact that complex types can't seem to be reused without duplicated generated types.

Will the interop layer be deprecated and removed at some point with the goal of full new architecture adoption in the (near?) future or will the interop layer remain supported? We were intent and working on full adoption but these missing codegen features are a blocker for us, and we don't have the understanding of react native internals nor currently the time to contribute these features ourselves, so understanding if these might make it onto your roadmap towards interop feature parity at some point or not would be valuable to know for us.

GillesMoris-Dolby avatar Mar 17 '25 12:03 GillesMoris-Dolby

Will the interop layer be deprecated and removed at some point with the goal of full new architecture adoption in the (near?) future or will the interop layer remain supported?

We'll be sunsetting the Interop Layers sometime soon (we don't have an exact timeline just yet). Those Interop Layers rely on Legacy Architecture infrastructure. That means that we're practically shipping 2 archs in every React Native app (with obvious cost on the final app bundles).

We'll also look into all of the remaining blockers (like this issue) before sunsetting those Interop Layers, so you'll be fine 👍

cortinico avatar Mar 17 '25 13:03 cortinico

For anyone wondering how to work around this issue, here is a possible one. Basically you can define your own UnsafeMixed:

export type UnsafeMixed<T> = T;

It's important to have this in a standalone/separate file, because RN codegen doesn't check where this UnsafeMixed is coming from. Now you can at least have some type sanity within TS, for example:

export type ViewPosition =
  | { top: Int32; left: Int32 }
  | { top: Int32; right: Int32 }
  | { bottom: Int32; right: Int32 }
  | { bottom: Int32; left: Int32 };

export interface NativeProps extends ViewProps {
  position: UnsafeMixed<ViewPosition >;
}

RN codegen will only see the UnsafeMixed. But now of course there is no native typesafety and the quite some glue code for the transformations is necessary. But at least it doesn't get you any unwanted defaults. As in this example where the occurrence of the keys controls the corner something gets positioned.

So would love to see this problem solved on the codegen level.

KiwiKilian avatar Aug 07 '25 20:08 KiwiKilian

This is a rather large issue for numeric props on view components.

In RN Camera Kit, where "zoom" and "maxZoom" is used a lot, we have a few numeric props and they are all optional. Codegen docs make it very clear that you can define optional properties (e.g. WithDefault<...>), but then neglects to show any example C++ output code, nor mention if it applies for both Fabric and regular modules. I looked at the tests which also do not cover optional values it seems. With WithDefault<Float, null> the best you can get is: Float{} zoom{}; which also doesn't really change or wrap the type.

For some properties with ranges like 0-Inf. it can be worked around by rewriting the library to use -1 in place of undefined, but for legitimately signed values like latitude/longitude, you have to make up a reserved value, which gets very hairy when you are using floats and doubles.

@KiwiKilian Does your workaround with for float, double, and ints?

scarlac avatar Aug 29 '25 18:08 scarlac

AFAIK it should work for all kinds of types.

Also looking into using WithDefault with -1. But WithDefault only seems to works on iOS. Can someone else confirm this? On Android the missing fields are omitted, making both platforms behave differently.

KiwiKilian avatar Aug 30 '25 20:08 KiwiKilian

Yeah the platforms behave differently. Android supports optional exactly as expected but iOS does not. Therefore, all libraries that use optional props for numbers on view components must rewrite to using these hacks or other hacks or drop New Arch support or drop iOS support.

scarlac avatar Aug 30 '25 21:08 scarlac

That is also the reason why we are currently holding off on converting our RN SDK's to a full architecture setup. Most of our API properties and methods contain optional values that are only processed when defined. Each with their own set of possible ranges, making standard 'default' values not trivial and a source for mistakes. All involved languages (TS, C++, ObjC, Swift, Kotlin, ...) have a way to handle optionals. The only part that is missing is the method and prop conversion done by codegen...

wvanhaevre avatar Sep 01 '25 07:09 wvanhaevre

https://github.com/facebook/react-native/pull/54724 Possible one step forward? Not sure, how much it solves.

KiwiKilian avatar Dec 13 '25 16:12 KiwiKilian

Thanks for the update! I'll have a look at the latest developments...

wvanhaevre avatar Dec 15 '25 08:12 wvanhaevre