Hook up fabric view component codegen #7759

acoates-ms · 2021-05-11T23:47:44Z

This PR hooks up a basic ActivityIndicatorViewComponent. This is the first view component implemented that uses codegen in core, so this PR also adds the logic to run codegen for view components.

Microsoft Reviewers: Open in CodeFlow

asklar · 2021-05-12T00:11:18Z

vnext/Microsoft.ReactNative/Fabric/ActivityIndicatorComponentView.h

+#include "ViewComponentView.h"
+
+#pragma warning(push)
+#pragma warning(disable : 4244 4305)


note 4244 is sdl, is this one that's already being fixed in core?

Yes there is already a PR out for this one.

asklar · 2021-05-12T00:14:28Z

vnext/codegen/react/components/rnwcore/ComponentDescriptors.h

@@ -0,0 +1,30 @@
+
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.


is this right?

This file is codegen'd. Given that this same codegen is going to be used for external components, I'll look at creating a PR in core to adjust this comment.
I agree it doesn't make sense to generate this:
facebook/react-native#31516

thanks. Separately, if this file is codegen'd, shouldn't be named .g.h?

The name of the file is similarly picked by the facebook codegen.
https://github.com/facebook/react-native/blob/b2dbde36b8f9cb69b1a1f9061c7f19c895d0c317/packages/react-native-codegen/src/generators/components/GenerateComponentDescriptorH.js#L52
Maybe we should look into seeing if thats something they'd be interested in changing.

asklar · 2021-05-12T16:55:41Z

vnext/codegen/react/components/rnwcore/EventEmitters.h

+ struct OnRequestClose {
+
+ };
+
+ struct OnShow {
+
+ };
+
+ struct OnDismiss {
+
+ };


are these structs and the other ones what we'd call event args? If so, can they be named accordingly? e.g. RequestCloseEventArgs

That's a window-ism. These are xplat classes from FB's codegen. If/when we do a higher level winrt projection of these we'd want to use that naming convention.

asklar · 2021-05-12T17:00:56Z

vnext/codegen/react/components/rnwcore/Props.cpp

+ progressImage(convertRawProp(rawProps, "progressImage", sourceProps.progressImage, {})),
+ trackImage(convertRawProp(rawProps, "trackImage", sourceProps.trackImage, {}))
+ {}
+AndroidSwipeRefreshLayoutProps::AndroidSwipeRefreshLayoutProps(


is this right? should we have android properties in RNW? is there something that can be queried to filter out platform-specific apis?

There is a property on the original definition of the component. Ex: RCTInputAccessoryView is marked as not being supported in android. Using that would be annoying though, since we'd need to update / fork core in order to mark things as being / not being supported in windows.
https://github.com/facebook/react-native/blob/b2dbde36b8f9cb69b1a1f9061c7f19c895d0c317/Libraries/Components/TextInput/RCTInputAccessoryViewNativeComponent.js#L25

I'm assuming that all these extra props classes etc will be stripped from the final ship dll since they are not referenced anywhere.

asklar · 2021-05-12T17:16:27Z

vnext/codegen/react/components/rnwcore/Props.h

+ enum ModalHostViewSupportedOrientations const rhs) {
+ lhs = lhs | static_cast<ModalHostViewSupportedOrientationsMask>(rhs);
+}
+


note you can do this with DEFINE_ENUM_FLAG_OPERATORS

alternately, consider just having the one set of operators with

asklar · 2021-05-12T17:19:31Z

vnext/codegen/react/components/rnwcore/Props.h

+ SafeAreaViewProps(const SafeAreaViewProps &sourceProps, const RawProps &rawProps);
+
+#pragma mark - Props
+


what are these pragmas? it doesn't look like msvc understands it?

I wonder if this should be #pragma region. Google says this may be ObjC specific.

yes, pragma region/endregion would be understood by VS/msvc

packages/@rnw-scripts/format-files/src/formatFiles.ts

NickGerleman · 2021-05-12T17:32:24Z

packages/@react-native-windows/codegen/src/Cli.ts

@@ -151,31 +162,70 @@ function generate(
 */

 const generateNM2 = createNM2Generator({namespace: argv.namespace});
+ const generatorPropsH = require('react-native-tscodegen/lib/rncodegen/src/generators/components/GeneratePropsH')


FYI that we're still on an older version of tscodegen/codegen. Zihan published a new version, with some possible changes, but I hadn't pulled it into RNW yet.

yeah. I keep wondering if we are going to need to pull tscodegen into rnw repo. Right now it seems to be ok with the older version. but it does worry me.

Do we have RN-core publishing the codegen pacakge in daily builds?

Nope. Original plan was for me to hookup nightlies for monorepo packages, but code stopped being pulled into separate packages so the urgency dropped off. There is a ready-made tscodegen that is newer than what we have rn though.

packages/@react-native-windows/codegen/src/Cli.ts

NickGerleman · 2021-05-12T17:38:30Z

vnext/Microsoft.ReactNative/Microsoft.ReactNative.vcxproj

@@ -466,6 +467,11 @@
 <ClCompile Include="$(ReactNativeDir)\ReactCommon\react\renderer\textlayoutmanager\TextMeasureCache.cpp" />
 <ClCompile Include="$(ReactNativeDir)\ReactCommon\react\renderer\uimanager\UIManager.cpp" />
 <ClCompile Include="$(ReactNativeDir)\ReactCommon\react\renderer\uimanager\UIManagerBinding.cpp" DisableSpecificWarnings="4389;4715;%(DisableSpecificWarnings)" />
+ <ClCompile Include="$(ReactNativeWindowsDir)codegen\react\components\rnwcore\Props.cpp" >
+ <ObjectFileName>$(IntDir)\codegenRnwCoreProps.obj</ObjectFileName>


OOC, why do we change the object file output for this translation unit?

It doesn't like having two cpp files with the same name. -- There is another Props.cpp in "$(ReactNativeDir)\ReactCommon\react\renderer\core\Props.cpp"

ghost · 2021-05-12T22:24:18Z

Hello @acoates-ms!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

acoates-ms added 6 commits May 11, 2021 16:22

Hook up view component codegen

f529397

Change files

290d9ce

Dont use relative imports

2d19aab

format

c7de3b4

Change files

d6ff54b

Run codegen for all the view components

836fb6f

acoates-ms requested a review from a team as a code owner May 11, 2021 23:47

acoates-ms added the Area: Fabric Support Facebook Fabric label May 11, 2021

asklar reviewed May 12, 2021

View reviewed changes

fix path

1c19cbc

asklar reviewed May 12, 2021

View reviewed changes

change to ignore codegen formatting

365d987

NickGerleman reviewed May 12, 2021

View reviewed changes

packages/@rnw-scripts/format-files/src/formatFiles.ts Outdated Show resolved Hide resolved

NickGerleman reviewed May 12, 2021

View reviewed changes

packages/@react-native-windows/codegen/src/Cli.ts Outdated Show resolved Hide resolved

NickGerleman reviewed May 12, 2021

View reviewed changes

acoates-ms added 8 commits May 12, 2021 10:42

x86 build fix

428620a

Add clang-format override file for codegen

835d3af

codegen uses LF

a1dc005

yse yargs required

05b3713

tmp

aec74a9

readd codegen'd files using LF

f33036b

Disable sort includes on the codegen files

12f17af

updated clang-format file

bdb4500

acoates-ms added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label May 12, 2021

NickGerleman approved these changes May 12, 2021

View reviewed changes

ghost merged commit 36e48e2 into microsoft:master May 12, 2021

acoates-ms deleted the activityind branch May 12, 2021 22:57

jonthysell added the New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric label Mar 7, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up fabric view component codegen #7759

Hook up fabric view component codegen #7759

acoates-ms commented May 11, 2021 •

edited by ghost

Loading

asklar May 12, 2021

acoates-ms May 12, 2021

asklar May 12, 2021

acoates-ms May 12, 2021

asklar May 12, 2021

acoates-ms May 12, 2021

asklar May 12, 2021

acoates-ms May 12, 2021

asklar May 12, 2021

acoates-ms May 12, 2021

asklar May 12, 2021

asklar May 12, 2021

NickGerleman May 12, 2021

asklar May 12, 2021

NickGerleman May 12, 2021

acoates-ms May 12, 2021

NickGerleman May 12, 2021

NickGerleman May 12, 2021

acoates-ms May 12, 2021

ghost commented May 12, 2021

		@@ -0,0 +1,30 @@

		/**
		* Copyright (c) Facebook, Inc. and its affiliates.

		SafeAreaViewProps(const SafeAreaViewProps &sourceProps, const RawProps &rawProps);

		#pragma mark - Props

Hook up fabric view component codegen #7759

Hook up fabric view component codegen #7759

Conversation

acoates-ms commented May 11, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 12, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

acoates-ms commented May 11, 2021 •

edited by ghost

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.