Make Microsoft.Toolkit.Uwp.Notifications linker-friendly

[Sergio0694]

Closes #4552

PR Type

What kind of change does this PR introduce?

• Optimization

What is the current behavior?

The Microsoft.Toolkit.Uwp.Notifications uses a lot of reflection and completely breaks down when trimming.

Additionally, the package is trying to bundle an .rd.xml file for UWP to preserve metadata, but this doesn't really seem to be working anyway. Using the library in an app with trimming enabled will still cause all notifications to be broken.

What is the new behavior?

The Microsoft.Toolkit.Uwp.Notifications package is fully trimmable (as reflection is no longer used).

PR Checklist

Please check if your PR fulfills the following requirements:

• Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
• Based off latest main branch of toolkit
• Tested code with current supported SDKs
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

[ghost]

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[Arlodotexe]

Love seeing improvements like this! Couldn't spot any issues, but let's get another pair of eyes on this to make sure we didn't miss anything and it's all working :)

[michael-hawker]

Need to track down new notifications PM to coordinate as they've owned this in the past. Following up with @marb2000 to find out who this is.

[andrewleader]

Neat changes! As the original author of this code, from a quick 2-minute skim over the code changes, it seems like an okay change (and we have sufficient unit test coverage that if the unit tests are passing I'm quite confident everything is still working).

I'll ping the new PM owner of notifications, Vaheeshta, and get her added to this pull request.

[Nirmal4G]

This is a welcome change. It was on my to do list but I was told there'd be a major refactoring in the future and integration into Reunion. So, I didn't bother to take it up. Glad to see this done!

[michael-hawker]

@vaheeshta looks like we forgot to ping your account on this, mind taking a look and letting us know if you have any concerns? Thanks!

[vaheeshta]

Changes look good. @loneursid to take a look as well.

[Sergio0694]

Hey @loneursid, did you have a chance to take a look? We'd like to potentially push out a small hotfix once this is merged, so we can reference the new linker-friendly version in the Microsoft Store and stop blocking trimming for this package 😄

[loneursid]

@Sergio0694, thanks for the ping, this totally fell off my radar. Will take a look shortly.

[Sergio0694]

That would be awesome, thank you! 😄

[loneursid]

@Sergio0694 - at a glance the changes look good but I wanted to build the project and run the tests. I can clone the main repo, build it and run the tests but I could do that with yours. I can clone your fork but it fails to build. Am I missing something?

[Sergio0694]

Mmh that's weird, I'm also getting build errors now:

They seem unrelated though, eg. the .NET Framework 4.6.1 TFM is out of support and I don't have it installed. The weird thing though is that checking out to 7eed078 (last commit from main), I can in fact build just fine. And the CI is also building just fine. I'm not really sure how the changes here could be causing that, especially because I didn't change anything related to TFMs...

I'll take a look 🤔

[Sergio0694]

@loneursid I can build just the Notifications project and run all tests, which pass:

Can you at least repro this on your end as well?

[Sergio0694]

Actually trying this again, I get the same exact build errors from main too. I don't think this is related to this PR 🤔
I can checkout main, build the solution, and I still get the two errors from the XamlIsland test project.
...Sometimes though main still builds fine even though I still have the error showing up in the errors window.

@michael-hawker are there any known build issues locally for those? Can you build the solution? Like, it seems there's something wonky going on with the XamlIsland test project, but I don't see how it could be related to this PR at all, since it doesn't touch anything outside of the notification APIs. This is a red herring and unrelated from the changes in this PR, no?

UPDATE: after installing the .NET Framework 4.6.2 SDK and targeting pack, I can build this branch too.
@loneursid to double check, if you build main and it succeeds, do you still also see an error in the errors window?

[loneursid]

The code builds and all tests are passing.
Did not spot anything egregious in the change list

[Sergio0694]

Thank you for taking a look @loneursid! 😄

I'll do another end to end test pass with a preview build of the Microsoft Store and merge this if everything looks good 🚀

[michael-hawker]

Thanks @Sergio0694 yeah the 4.6.2 SDK targeting pack is a build requirement still, glad you were able to figure out that was the underlying issue. I haven't tried to build the main repo for a while now as we've been focused on Labs.

I'll merge this in, and then we can figure out plans for strategy later, though if it can wait until the beginning of August to figure out hotfix that'd be ideal as we're trying to close down on the initial release of Labs.

[michael-hawker]

Oops, @Sergio0694 I misread your comment as have already doing that step and merged... let me know if you find an issue and we'll figure out a plan to rectify if so. Sorry, juggling many things right now! 🙈

[Sergio0694]

Tested in the Store again, everything looking good! 🚀

Ship it!