-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Remove our dependency on Microsoft.Toolkit.Win32.UI.XamlApplication #14520
Conversation
We used to require it. Now it looks like we don't, so merge its init code into our App.
} | ||
} | ||
|
||
void App::Close() |
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.
behavior that was originally trapped in the toolkit application is herein merged (same for TerminalApp)
namespace winrt::SampleApp::implementation | ||
{ | ||
App::App() | ||
{ | ||
// This is the same trick that Initialize() is about to use to figure out whether we're coming |
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 comment is still relevant
🚀 thanks a lot for these changes, this will bring us closer to properly supporting a Windows Terminal build option without the dynamic visual studio runtime! 🙏🪄 |
@msftbot merge this in 10 minutes |
Hello @carlos-zamora! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
I trust this change about as far as I could throw it... so I'd love to get a good validation pass on the release build after it lands :) |
…14520) We originally needed this library (or a separate DLL in our own project) to handle hooking up the XAML resource loader to the providers that our application needed. It was introduced in its nascent form in 2019, in a PR titled "Make XAML files work." It appears we no longer need it, and the provider hookup is being handled by our `AppT2` base class override. I've tested this in Windows 10 Vb running unpackaged, and it seems to work totally fine. Crazy. Removing this dependency saves us a couple hundred kilobytes on disk and removes one consumer of the App CRT from our package. (cherry picked from commit 68cce10)
…14520) We originally needed this library (or a separate DLL in our own project) to handle hooking up the XAML resource loader to the providers that our application needed. It was introduced in its nascent form in 2019, in a PR titled "Make XAML files work." It appears we no longer need it, and the provider hookup is being handled by our `AppT2` base class override. I've tested this in Windows 10 Vb running unpackaged, and it seems to work totally fine. Crazy. Removing this dependency saves us a couple hundred kilobytes on disk and removes one consumer of the App CRT from our package. (cherry picked from commit 68cce10)
FWIW, I have been running a XAML Islands app in prod without that dependency for ~2 years :) I've never liked that dependency (and the forced App CRT dep it implies) so quickly worked getting rid of it once I realized it never did much. |
…14520) We originally needed this library (or a separate DLL in our own project) to handle hooking up the XAML resource loader to the providers that our application needed. It was introduced in its nascent form in 2019, in a PR titled "Make XAML files work." It appears we no longer need it, and the provider hookup is being handled by our `AppT2` base class override. I've tested this in Windows 10 Vb running unpackaged, and it seems to work totally fine. Crazy. Removing this dependency saves us a couple hundred kilobytes on disk and removes one consumer of the App CRT from our package. (cherry picked from commit 68cce10)
@sylveon thanks so much for that. Gives me the confidence to backport this to stable and remove one more variable from our troubleshooting pipeline :) |
🎉 Handy links: |
🎉 Handy links: |
Could removing this have caused #14729? I'd reckon it's possible. Maybe there's an interplay between this and the FWP/Win11 version of the package. |
We originally needed this library (or a separate DLL in our own project)
to handle hooking up the XAML resource loader to the providers that our
application needed. It was introduced in its nascent form in 2019, in a
PR titled "Make XAML files work."
It appears we no longer need it, and the provider hookup is being
handled by our
AppT2
base class override. I've tested this in Windows10 Vb running unpackaged, and it seems to work totally fine. Crazy.
Removing this dependency saves us a couple hundred kilobytes on disk and
removes one consumer of the App CRT from our package.