-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Xaml support prototype #14811
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
base: main
Are you sure you want to change the base?
Xaml support prototype #14811
Conversation
@@ -83,6 +83,17 @@ | |||
"boost": "[1.83.0, )" | |||
} | |||
}, | |||
"microsoft.reactnative.xaml": { |
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'm not sure why this entry is here -- "microsoft.reactnative.xaml" isn't a package, is it?
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.
lock files include project dependencies
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.
OK, this should go away when we remove the m.rn.xaml.vcxproj then.
@@ -39,6 +41,9 @@ _Use_decl_annotations_ int CALLBACK WinMain(HINSTANCE instance, HINSTANCE, PSTR | |||
RegisterAutolinkedNativeModulePackages(settings.PackageProviders()); | |||
// Register any native modules defined within this app project | |||
settings.PackageProviders().Append(winrt::make<CompReactPackageProvider>()); | |||
// TODO: Can we make apps register this automatically? (e.g. with autolinking) | |||
// TODO: But ideally we don't load the M.RN.Xaml.dll at all if we're not using Xaml. | |||
settings.PackageProviders().Append(winrt::Microsoft::ReactNative::Xaml::ReactPackageProvider()); |
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.
TODO: I don't know how to make this automatically happen with autolinking or whatever. Can the xaml-calendar-view package do this somehow? OTOH, if the app's not using Xaml, I don't want it to load Microsoft.ReactNative.Xaml.dll. Not sure the best way to handle 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.
We would probably want to update autolinking to conditionally add the winrt::Microsoft::ReactNative::Xaml::ReactPackageProvider()
to be loaded in the generated AutoLinkedNativeModules.g.cpp
file.
We can either detect it needs to be there during autolinking (modules might register that they need it) and inject it there, OR we could just always put it in, behind a #ifdef RNW_NEW_ARCH_XAML_SUPPORT
, powered by an aforementioned RnwNewArchXamlSupport
variable.
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.
Another solution: if XamlHost were in its own package, the existing autolinking logic should make this work.
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.
DECISION: We decided to remove the Microsoft.ReactNative.Xaml.dll and move XamlHost into Microsoft.ReactNative.dll. This should resolve this problem.
); | ||
} | ||
|
||
export { CalendarView, RawCalendarView }; |
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 idea here is that the CalendarView object is already wrapped in a XamlHost, so is all ready to be put into a JSX tree. RawCalendarView is not. In the future maybe we make it possible to group multiple "Raw" elements under the same XamlHost.
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 you had a tree of Xaml elements, would you expect something like this? (maybe not supporting layout panels with multiple children, but other controls that have 1 child)
<XamlHost>
<RawStackPanel>
<RawButton />
</RawStackPanel>
</XamlHost>
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 we could support this without too much work if we want to. I haven't spent a lot of time exploring that though.
}; | ||
|
||
[webhosthidden] | ||
interface IXamlControl |
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.
Implement this if your wrapping a Xaml object and want to be hostable in a XamlHost.
@@ -83,6 +83,17 @@ | |||
"boost": "[1.83.0, )" | |||
} | |||
}, | |||
"microsoft.reactnative.xaml": { |
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.
lock files include project dependencies
@@ -18,5 +18,13 @@ | |||
<Name>Microsoft.ReactNative</Name> | |||
<Private Condition="'$(ConfigurationType)' != 'Application'">false</Private> | |||
</ProjectReference> | |||
|
|||
<!-- TODO: Should we only include this project ref conditionally? --> |
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 files is used by any external app/lib that tries to build RNW from source - and for NewArch we're trying to get everyone to build on the NuGets by default. The only apps that are building NewArch from source should be the ones inside this repo.
I would definitely put this in a conditional behind a new variable, maybe RnwNewArchXamlSupport
? We could also use that variable to control a package reference (depending on if we publish this as a separate nuget or just as part of the existing nuget)
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.
peferably XamlApplication.idl?
@@ -39,6 +41,9 @@ _Use_decl_annotations_ int CALLBACK WinMain(HINSTANCE instance, HINSTANCE, PSTR | |||
RegisterAutolinkedNativeModulePackages(settings.PackageProviders()); | |||
// Register any native modules defined within this app project | |||
settings.PackageProviders().Append(winrt::make<CompReactPackageProvider>()); | |||
// TODO: Can we make apps register this automatically? (e.g. with autolinking) | |||
// TODO: But ideally we don't load the M.RN.Xaml.dll at all if we're not using Xaml. | |||
settings.PackageProviders().Append(winrt::Microsoft::ReactNative::Xaml::ReactPackageProvider()); |
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 would probably want to update autolinking to conditionally add the winrt::Microsoft::ReactNative::Xaml::ReactPackageProvider()
to be loaded in the generated AutoLinkedNativeModules.g.cpp
file.
We can either detect it needs to be there during autolinking (modules might register that they need it) and inject it there, OR we could just always put it in, behind a #ifdef RNW_NEW_ARCH_XAML_SUPPORT
, powered by an aforementioned RnwNewArchXamlSupport
variable.
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 don't want another file here, we already have one at the repo root to keep all of the projects aligned.
<ClInclude Include="pch.h" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<Midl Include="Class.idl" /> |
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.
Out of date files
vnext/Microsoft.ReactNative.Xaml.sln
Outdated
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.
Not sure if we need this to be it's own solution - maybe fine for now, will depend on how we want to package the dll
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.
DECISION: We decided to remove Microosft.ReactNative.Xaml.dll entirely. So this sln will go away too.
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 XamlCalendarView doesn't have any non-UI functionality, you can probably remove the struct here.
<AppContainerApplication>false</AppContainerApplication> | ||
<ApplicationType>Windows Store</ApplicationType> | ||
<ApplicationTypeRevision>10.0</ApplicationTypeRevision> | ||
<WindowsTargetPlatformVersion Condition=" '$(WindowsTargetPlatformVersion)' == '' ">10.0.22621.0</WindowsTargetPlatformVersion> |
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.
Don't want to hardcode the SDK versions.
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.
Does having this type here preclude app authors from having their own Xaml app? I.e. A classic WinAppSDK app with it's own App, using a ReactNativeIsland, to put RNW in just part of the UI, then have that RNW also load Xaml?
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 is possible, but the app would need to have its own Application object subclass the RNW XamlApplication. WinAppSDK also only supports one version in the process at a time.
import * as React from 'react'; | ||
import RawCalendarView from './CalendarViewNativeComponent'; | ||
|
||
import XamlHost from 'react-native-windows/Libraries/Components/Xaml/XamlHost'; |
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 an awkward import into a deep path. Should XamlHost be a separate package?
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.
DECISION: Let's fold the Xaml logic into the M.RN.dll. We still want to be careful that Xaml-specific code doesn't get mixed in with the core logic.
Here's the idea:
Microsoft Reviewers: Open in CodeFlow