Skip to content
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 Microsoft.AspNetCore.Components.Web dependency #52

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

Dreamescaper
Copy link

@Dreamescaper Dreamescaper commented Nov 23, 2020

Replace Microsoft.AspNetCore.Components.Web dependency with Microsoft.AspNetCore.Components.Forms.
That would allow to avoid unnecessary dependencies when using this library with non-Web Blazor implementations (e.g. https://github.com/xamarin/MobileBlazorBindings).

I've also removed _Imports.razor file, is that needed for some reason?

@ryanelian
Copy link
Owner

ryanelian commented Nov 24, 2020

Hello and thanks for the PR!

Replace Microsoft.AspNetCore.Components.Web dependency with Microsoft.AspNetCore.Components.Forms.
That would allow to avoid unnecessary dependencies when using this library with non-Web Blazor implementations (e.g. https://github.com/xamarin/MobileBlazorBindings).

That is very interesting. I didn't know that my library can be run on Xamarin Blazor. Amazing!

Before I accept this PR, I just want to double check, Have you actually tried running your branch on Xamarin Blazor on a real mobile device?

I've also removed _Imports.razor file, is that needed for some reason?

This library was originally written on .NET Core Razor Class Library project 3.0, which added that file by default. I've never touched that file personally, but left it alone because why fix things that are not broken?

@Dreamescaper
Copy link
Author

That is very interesting. I didn't know that my library can be run on Xamarin Blazor. Amazing!

Before I accept this PR, I just want to double check, Have you actually tried running your branch on Xamarin Blazor on a real mobile device?

Yes, I already use this library on my project, the only thing that I had to define edit controls which notify parent EditContext about changes.
I've submitted PR with such controls, you can take a look at screen recording from my phone there: dotnet/MobileBlazorBindings#243.

I haven't really checked running it with this branch, because Xamarin does not support net5.0 (they plan to add net6.0), so I cannot use latest version :( .

The only thing that I haven't thought about when created this PR - while Components.Web dependency is not required for this library (well, apart from nameof usage), removing this dependency would be a breaking change for projects, where is no direct reference to Components.Web (i.e. it is added transitively).
Not sure if that's something to worry about.

@ryanelian
Copy link
Owner

Yes, I already use this library on my project, the only thing that I had to define edit controls which notify parent EditContext about changes.
I've submitted PR with such controls, you can take a look at screen recording from my phone there: dotnet/MobileBlazorBindings#243.

Truly remarkable. I can't wait to develop Blazor on Xamarin in near future.

I haven't really checked running it with this branch, because Xamarin does not support net5.0 (they plan to add net6.0), so I cannot use latest version :( .

I'll see what I can do to make the next version compatible with both .NET 5 and .NET Core 3.1

The only thing that I haven't thought about when created this PR - while Components.Web dependency is not required for this library (well, apart from nameof usage), removing this dependency would be a breaking change for projects, where is no direct reference to Components.Web (i.e. it is added transitively).
Not sure if that's something to worry about.

Logically it shouldn't be a breaking change since people don't usually use this library outside Blazor app / Razor class library.

But just to be safe, I'll publish the new package with changed dependency as version 5.0.0 later.


Alright, I'm convinced. I'll accept the PR into a development branch for me to play with. Thanks!

@ryanelian ryanelian changed the base branch from master to 5.0.0 November 25, 2020 03:46
@ryanelian ryanelian merged commit f778770 into ryanelian:5.0.0 Nov 25, 2020
@Dreamescaper Dreamescaper deleted the remove_web_dependency branch December 23, 2020 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants