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

Make the Resolve method public #31

Closed
IeuanWalker opened this issue Apr 25, 2024 · 10 comments · Fixed by #39
Closed

Make the Resolve method public #31

IeuanWalker opened this issue Apr 25, 2024 · 10 comments · Fixed by #39

Comments

@IeuanWalker
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
I'd like to be able to use the Resolve method directly in order to extend the functionality for things other than INavigation.

Describe the solution you'd like
Make certain methods public, or possibly even create a separate project for Mopups support and give it access to internal methods of this project.

Additional context
We use Mopups for our popups in our app. After a quick fiddle, I was able to get PageResolver to work with Mopups.

For example, I registered the popup and ViewModel like any other page -

builder.Services.AddTransient<AddPopup>();
builder.Services.AddTransient<AddReminderViewModel>();

builder.Services.UsePageResolver(new Dictionary<Type, Type>()
{
	{ typeof(AddPopup), typeof(AddReminderViewModel) }
});

I then made the Resolve<T> public, and was able to get this to work -

await MopupService.Instance.PushAsync(Resolver.Resolve<AddPopup>());

Obviously, I'd need to do quite a bit more work to get it to work with parameters.
Just seeing if you're open to something like this?

@IeuanWalker
Copy link
Collaborator Author

IeuanWalker commented Apr 25, 2024

@matt-goldman Actually it's quite straightforward.

No parameters

To inject a popup with no parameters, I just need the Resvole method made public -
https://github.com/matt-goldman/Maui.Plugins.PageResolver/blob/ff594dae8a38a20d365ca9748214a17df90aa052/src/Maui.Plugins.PageResolver/Resolver.cs#L63C5-L63C51

await MopupService.Instance.PushAsync(Resolver.Resolve<AddPopup>());

With parameters

To inject a popup with parameters, just need the ResolvePage<T> method made public (PopupPage is just a ContentPage so the current code works fine)

private static Page ResolvePage<T>(params object[] parameters) where T : Page

await MopupService.Instance.PushAsync((PopupPage)NavigationExtensions.ResolvePage<AddPopup>(123));

Might need to think of a way to avoid the cast though

@matt-goldman
Copy link
Owner

Sorry for the delay. I want to keep this private to avoid service locator behavior. I think what I'll do here is release a separate package, MopupsExtensions that lets you just use PushAsync with Mopups. Do you have any other use cases for making it public or would this work for you?

@IeuanWalker
Copy link
Collaborator Author

@matt-goldman thanks, ye that will work fine :)

I did have to change something in the resolve method as I was getting a weird exception, I will work out the changes I made sometime next week

@IeuanWalker
Copy link
Collaborator Author

@matt-goldman this was the exception i mentioned above -
image

It comes from this line - https://github.com/matt-goldman/Maui.Plugins.PageResolver/blob/88b83a2c16495f00afb0eb45dd5ffd65975205b5/src/Maui.Plugins.PageResolver/NavigationExtensions.cs#L88C8-L88C105

Changing the code from -

var resolvedPage = ActivatorUtilities.CreateInstance<T>(serviceProvider, typeof(T), parameters);
return resolvedPage;

To -

return ActivatorUtilities.CreateInstance<T>(serviceProvider, parameters);

Im not too familiar with the internal DI methods so not sure what the difference is, but after I made that change it all started to work. The code that caused the exception is pretty straightforward -

  • Page (popup) constructor -
    image
  • Opening the popup
    image
  • My extension methods to get PageResolver to work with mopopups
    image

@matt-goldman
Copy link
Owner

Hi @IeuanWalker, sorry this has taken so long!

I've got #39 open which should give you what you need. Slight variation on yours, it's an extension on IPopupNavigation from the Mopups library.

I've published the nuget already, when you get the chance please test and let me know what you think. I'll update the wiki soon too.

@IeuanWalker
Copy link
Collaborator Author

IeuanWalker commented Nov 4, 2024

@matt-goldman i cloned your repo to update my local changes to make sure everything worked before switching to the NuGets, and all worked great.

So I removed the local copy and installed the NuGets instead -
image

But am now getting source generator errors -
image

And far as im aware im not using your SG stuff -

  • PageResolver.Autoreg nuget not installed
  • Not calling UseAutodependencies()

@matt-goldman matt-goldman reopened this Nov 4, 2024
@matt-goldman
Copy link
Owner

Hey @IeuanWalker, thanks for testing this. Sorry this has happened - this is because I tried to improve the convenience of auto dependencies. It runs whether or not you call it (it has to in order to generate the UseAutodependencies method), but I think there needs to be a flag to trigger it. I'll need to give this some thought, but I think following the CommunityToolkit approach, where you either inherit a base class (I'll use an interface) or add an attribute to trigger the source generator is the way to go.

That will need to be in a future version though, if I do it now it will be a breaking change and I think that needs some advance warning. In the meantime I'll release a version that does the opposite - I'll give you an interface and attribute (you can pick which one you want to use) that marks it for exclusion, and this will terminate the source generator.

Immediate version:

public static class MauiProgram : INoAutoDependencies

// or, pick whichever

[NoAutoDependencies]
public static class MauiProgram

future:

public static class MauiProgram : IUseAutoDependencies

// or, pick whichever

[UseAutoDependencies]
public static class MauiProgram

I'll push the immediate update today and open a discussion for the future version. Sorry I know it's not ideal, but hopefully that works for you.

@IeuanWalker
Copy link
Collaborator Author

IeuanWalker commented Nov 4, 2024

Thanks @matt-goldman, a DataAnnotation will be fine :)

@matt-goldman
Copy link
Owner

Done in #40. v2.5.2 processing with Nuget now, should be available any minute. I've tested and it works for me, please let me know how you go.

Thanks! 🙂

@IeuanWalker
Copy link
Collaborator Author

@matt-goldman thanks, that all works with the NuGets now :)

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 a pull request may close this issue.

2 participants