-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fabric component: codegen drops optional state for component properties and in events properties #49920
Comments
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. |
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. |
Checked with 0.76.7 and issue still remains. |
@wvanhaevre Thanks for opening the issue. We will look into it as soon as we can. |
Codegen definitely has support for optional props: react-native/packages/react-native-codegen/src/generators/modules/GenerateModuleH.js Lines 141 to 228 in c04b4a9
See the various |
@cortinico , just to make sure, adding the ? To the spec description is enough to make the property optional, right? i.e.
makes the 2 last string optional and should appear on the C++ side as std::optional<std::string> ? |
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 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 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 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. |
We are indeed looking for a way to get this generated:
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:
We still get as generated struct:
|
@wvanhaevre are you using typescript to define your interfaces? 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. |
Yep... components does not support std::optional currently. @cortinico the file you were looking at is in the |
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. |
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:
Codegen creates:
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
Stacktrace or Logs
Reproducer
https://github.com/wvanhaevre/NewArchReproducer/tree/reproducer/no-optionals
Screenshots and Videos
No response
The text was updated successfully, but these errors were encountered: