auth0 / auth0-oidc-client-net

OIDC Client for .NET Desktop and Mobile applications
https://auth0.github.io/auth0-oidc-client-net/
Apache License 2.0
84 stars 49 forks source link

Updated TargetFramework to .NET5 in Auth0.OidcClient.WPF and WPF test APP #265

Closed joseangelmt closed 1 year ago

joseangelmt commented 1 year ago

Changes

I have changed the target framework from .NET Framework 4.6.2 to .NET 5 in both the WPF component and the WPF test application.

The change is made to be compatible with .NET 5.0 (which is now deprecated). Changing it to .NET 6 is as simple as changing a number. I'm not clear if it's better to do it for .NET 5 or for .NET 6 or .NET 7, so when in doubt, I've done it for .NET 5 to add maximum backwards compatibility.

To make this change it has been necessary to change the WebView component to the new WebView2 component (replacing the Nuget package Microsoft.Toolkit.Wpf.UI.Controls.WebView by Microsoft.Web.WebView2).

In addition, I had to create the WebWindow class that inherits from Window and overloads the OnContentRendered method so that WebView2 is initialized correctly, and I have simplified the logic of the WebViewBrowser class apart from making the factory of windows of the WebViewBrowser class return instances of WebWindow.

Testing

To test the changes simply compile and run the WPF test application.

Checklist

frederikprijck commented 1 year ago

Thanks for the PR.

This PR breaks existing users and makes the SDK unusable on .NET framework, which is still supported and we have no plans to remove this.

We should ensure we support both .NET Framework as well as .NET6+, both for WPF as well as WinForms.

This integration is a bit more complicated then it seems, because of the above requirement. We are planning to improve this in the near future.

That said, happy to review a PR that ensures compatibility with both .NET Framework and .NET 6, 7 and 8 if that's something you believe you want to do, but this isn't going to be a simple PR so I woud argue it's something we are better of picking up ourselves (which we are planning to in the near future).

Having said that, I believe you want to have a look at https://github.com/IdentityModel/IdentityModel.OidcClient.Samples/blob/main/WpfWebView2/WpfWebView2/WpfEmbeddedBrowser.cs to have an example of WebView2 integration that does not need OnContentRendered, which should already work in your application using the SDK in its current state, see our sample application using .NET 6 with our WPF SDK (note this dates from before the official sample from IdentityModel.OidcClient, so the browser implementation differs a bit).

Long story short, I believe nothing stops you from already using our SDK with .NET 6 (with a bit of manual work), and supporting it out of the box in our SDK is way more complicated than this PR so I would prefer to tackle this ourselves.

Additionally, even though we appreciate any contribution, please open issues before contributing PRs of this size and impact, to avoid putting a lot of effort on things that might not be merged.

joseangelmt commented 1 year ago

Hello @frederikprijck,

I apologize because I had read all the documents to make a PR but I had not read the CONTRIBUTING.md file of the repository itself which is where it is explained that compatibility cannot be broken.

I have created another PR that simply adds the functionality for .NET6+ without breaking existing compatibility. I have also modified the .nuspec files to be able to create updated NuGet packages and I have it running in my WPF application with .NET7.

Best regards and thank you very much.