-
Notifications
You must be signed in to change notification settings - Fork 467
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
feat(windows): add New Architecture support #1147
base: main
Are you sure you want to change the base?
feat(windows): add New Architecture support #1147
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There are some changes here that seem incompatible with old architecture. Have you checked that it still works?
- There are a lot of unrelated changes that need to be reverted. Especially after chore: bump
react-native
to 0.75 #1146 lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check in generated code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a duplicate of what's in packages/default-storage/windows/ReactNativeAsyncStorage/codegen
- are both copies necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format
travels up from the current path looking for a config file. These two are in different paths so I'd say we need both:
packages/default-storage/windows/ReactNativeAsyncStorage/codegen/.clang-format
packages/default-storage/windows/code/codegen/.clang-format
^~~ first common dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something seems up if we have two copies of the generated code. hats not right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the commit history, I think the two copies came from two different commits. The first one was from when I think I ran codegen-windows
independently: codegen-windows commit
The second one was from this commit where I I ran it again: second codegen commit. I think this was user error vs an issue with the template.
@@ -20,4 +20,4 @@ namespace winrt::ReactNativeAsyncStorage::factory_implementation | |||
struct ReactPackageProvider | |||
: ReactPackageProviderT<ReactPackageProvider, implementation::ReactPackageProvider> { | |||
}; | |||
} // namespace winrt::ReactNativeAsyncStorage::factory_implementation | |||
} // namespace winrt::ReactNativeAsyncStorage::factory_implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
...ges/default-storage/windows/packages/Microsoft.Windows.CppWinRT.2.0.200316.3/.nupkg.metadata
Outdated
Show resolved
Hide resolved
I tried building with the old architecture following the below steps, and was able to successfully deploy the example app (after removing the generated code from
I changed the call to |
The 0.75 bump just merged. This PR should reduce in size significantly if you rebase now. |
2158b30
to
123b3fd
Compare
@@ -1,3 +1,4 @@ | |||
module.exports = { | |||
presets: ["module:@react-native/babel-preset"], | |||
plugins: ["@babel/plugin-transform-runtime"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? It should already be included by the preset.
} | ||
/> | ||
<Button | ||
testID="unsetDelegate_button" | ||
title="Unset native delegate" | ||
disabled={ | ||
!NativeModules["AsyncStorageTestSupport"] || | ||
!NativeModules["AsyncStorageTestSupport"].test_unsetDelegate | ||
TurboModuleRegistry.get('AsyncStorageTestSupport')?.test_unsetDelegate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check needs to be inverted back:
TurboModuleRegistry.get('AsyncStorageTestSupport')?.test_unsetDelegate | |
!TurboModuleRegistry.get('AsyncStorageTestSupport')?.test_unsetDelegate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to the Windows changes. Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to the Windows changes. Please revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated to the Windows changes. Please revert.
<ItemDefinitionGroup> | ||
<ClCompile> | ||
<PrecompiledHeader>Use</PrecompiledHeader> | ||
<PrecompiledHeaderFile>pch.h</PrecompiledHeaderFile> | ||
<PrecompiledHeaderOutputFile>$(IntDir)pch.pch</PrecompiledHeaderOutputFile> | ||
<WarningLevel>Level4</WarningLevel> | ||
<WarningLevel>Level3</WarningLevel> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we lower the warning level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was automatically done when running the cpp-lib template - should we raise it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's not make any unnecessary changes.
<ItemGroup> | ||
<ClInclude Include="..\code\pch.h" /> | ||
<ClInclude Include="pch.h" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we include another set of pch.*
? What are we doing about the old ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new pch.*
files were generated when I ran the cpp-lib
template. The old ones are just there, I haven't included them in the project file. Should they be okay to delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they're no longer used, we should clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this file? It doesn't look like it provides anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's basically an empty file. Can we delete both? I don't think AsyncStorage is ever going to include any resources.
...ages/default-storage/windows/packages/Microsoft.Windows.CppWinRT.2.0.200316.3/.signature.p7s
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be reverted. Please note that all text files must end with an EOL: https://www.baeldung.com/linux/files-end-with-newlines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this file. Now we have an unused import and a formatting issue.
<ItemGroup> | ||
<ClInclude Include="..\code\pch.h" /> | ||
<ClInclude Include="pch.h" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they're no longer used, we should clean up.
@@ -0,0 +1,2 @@ | |||
DisableFormat: true | |||
SortIncludes: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline to the end of file: https://www.baeldung.com/linux/files-end-with-newlines
</ItemGroup> | ||
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" /> | ||
<ImportGroup Label="ReactNativeWindowsTargets"> | ||
<Import Project="$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.targets" Condition="Exists('$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.targets')" /> | ||
<Import Project="$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.targets" Condition="'$(UseFabric)'!='true' And Exists('$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.targets')" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should work out how to use
microsoft/react-native-windows#13945
Do we have any variables available to the vcxproj for RNW version? -- Seems like that would be useful so that we could import Uwp.CppLib on older RNW versions, but the newer import on newer RNW versions.
If we dont have such a variable, we should add one to newer RNW versions, and we can use the lack of such a variable as indication to use the older build logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that logic is in Microsoft.ReactNative.CppLib.props and Microsoft.ReactNative.CppLib.targets and will be in 0.76. At some point we just have to say, RNW fabricis soft launching in 0.76, the template in 0.76 is the supported base and 0.76 is the supported min version for fabric. Projects are welcome to use UseFabric themselves to replicate the functionality for RNW < 0.76, but I don't want that in the template.
|
||
void ReactPackageProvider::CreatePackage(IReactPackageBuilder const &packageBuilder) noexcept | ||
{ | ||
AddAttributedModules(packageBuilder, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats the supported RNW version range of async-storage? -- Should we put the extra "true" args behind a version check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think this file is unused - the included file is https://github.com/react-native-async-storage/async-storage/pull/1147/files/b9f2e55486dfcbeb15f9dc6d0ab37e999913a553#diff-91c8b85a4a6cfde2508040988d613ec059855d3e4cca40423d0edc7478af9301
I'm using USE_FABRIC
to decide whether to use the true
arg or not - this allowed me to build on both old and new architecture. Looks like the extra ReactPackageProvider.*
files in this directory should be good to remove.
); | ||
const AsyncStorageTestSupport = TurboModuleRegistry | ||
? TurboModuleRegistry.get("AsyncStorageTestSupport") | ||
: NativeModules["AsyncStorageTestSupport"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TurboModuleRegistry already fallsback to NativeModules, so no need to check both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling TurboModuleRegistry directly, the tsc
check was throwing the following error: https://github.com/react-native-async-storage/async-storage/actions/runs/11295316739/job/31491051486#step:5:26. I tried changing the call to TurboModuleRegistry.get<TurboModule>
in case it was a typing issue, but same result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're getting typing issues because TurboModule
is a generic type, it doesn't contain the two methods that are being called further down. You need to first define the types:
type AsyncStorageDelegate = () => void;
type AsyncStorageTestSupport = {
test_setDelegate: (delegate: AsyncStorageDelegate) => void;
test_unsetDelegate: (delegate: AsyncStorageDelegate) => void;
};
Then use it like this:
const AsyncStorageTestSupport = TurboModuleRegistry.get<AsyncStorageTestSupport>("AsyncStorageTestSupport");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acoates-ms this commit should make use of microsoft/react-native-windows#13945 once that PR is backported into 0.76.
I assumed that if someone is building with a canary build, that it would be the latest canary so it will include template update - which is why the check for the new props and targets import is an Or
. Does the logic here look good?
package.json
Outdated
@@ -40,7 +40,8 @@ | |||
"@react-native/js-polyfills": "^0.75.0", | |||
"@react-native/normalize-colors": "^0.75.0", | |||
"@react-native/virtualized-lists": "^0.75.0", | |||
"find-babel-config/json5": "^2.1.1" | |||
"find-babel-config/json5": "^2.1.1", | |||
"react-native-windows": "^0.75.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add a resolution override. You can bump the package in yarn.lock
. Since it's already been bumped, you can remove this entry only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
<Import Project="PropertySheet.props" /> | ||
</ImportGroup> | ||
<ImportGroup Label="ReactNativeWindowsPropertySheets"> | ||
<Import Project="$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.props" Condition="Exists('$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.props')" /> | ||
<ImportGroup Label="ReactNativeWindowsPropertySheets" Condition="'$(ReactNativeWindowsCanary)'!='true' And '$([MSBuild]::VersionLessThan($(ReactNativeWindowsMinor), 76))'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$([MSBuild]::VersionLessThan($(ReactNativeWindowsMajor).$(ReactNativeWindowsMinor).$(ReactNativeWindowsPatch), '0.76.0'))
Otherwise if we ever get to a version 1.0, it would count as a version less than 76.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still fail to do the right thing if we publish a RNW 1.0.0
I think we want:
Condition="'$(ReactNativeWindowsCanary)'!='true' And $([MSBuild]::VersionLessThan('$(ReactNativeWindowsMajor).$(ReactNativeWindowsMinor).$(ReactNativeWindowsPatch)', '0.76.0'))""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Import Project="$(ReactNativeWindowsDir)\PropertySheets\external\Microsoft.ReactNative.Uwp.CppLib.props" Condition="'$(UseFabric)'!='true' And Exists('$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Uwp.CppLib.props')" /> | ||
<Import Project="$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Composition.CppLib.props" Condition="'$(UseFabric)'=='true' And Exists('$(ReactNativeWindowsDir)\PropertySheets\External\Microsoft.ReactNative.Composition.CppLib.props')" /> | ||
</ImportGroup> | ||
<ImportGroup Label="ReactNativeWindowsPropertySheets" Condition="'$(ReactNativeWindowsCanary)'=='true' Or '$([MSBuild]::VersionGreaterThanOrEquals($(ReactNativeWindowsMinor), 76))'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a way to do an if else, I think its called choice or something. You should do that, so that we only include one set of the props files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the When
documentation, we cannot add ImportGroup
as a child element of a <When>
statement. With the current implementation, I don't think both sets of props files will be included since both conditions won't evaluate to true at the same time.
</ClCompile> | ||
<Link> | ||
<AdditionalDependencies>shell32.lib;user32.lib;windowsapp.lib;%(AdditionalDependenices)</AdditionalDependencies> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't windowsapp.lib a UWP thing? Not sure that that should be included in fabric apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also think this is from the cpp-lib template - not sure whether we should keep it or not. @jonthysell do you have any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still in the new arch lib template. Maybe it isn't necessary for new arch, but it might still be for old arch.
@@ -113,16 +131,24 @@ | |||
<PreprocessorDefinitions>NDEBUG;%(PreprocessorDefinitions)</PreprocessorDefinitions> | |||
</ClCompile> | |||
</ItemDefinitionGroup> | |||
<ItemDefinitionGroup> | |||
<ClCompile> | |||
<PreprocessorDefinitions Condition="'$(UseFabric)'=='true'">USE_FABRIC;%(PreprocessorDefinitions)</PreprocessorDefinitions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be part of the props file? - Or do all apps have to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonthysell - Is this something that we can move into one of the shared props files, so that not every package needs to include this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I plan on having this ready for 0.76.0, though it will probably be a new #define for customers (like RNW_NEW_ARCHITECTURE) : microsoft/react-native-windows#13929
Summary
Adds support for the New Architecture to
async-storage
. Currently, windows support for the New Architecture is experimental and may not be complete.Test Plan
On a Windows machine, run the following: