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

Allow users to specify resolution method for handlers, effects, and services #1870

Merged
merged 7 commits into from Feb 26, 2018

Conversation

@hartez
Copy link
Member

commented Feb 13, 2018

Description of Change

Forms provides no option for users to construct and provide their own instances of handlers, effects, or services in a Forms application; instead, users must rely on Activator.CreateInstance. This greatly limits user control over the construction and lifetime of components.

This change provides a way for the user to inject their own dependency resolution method into Forms. When Forms needs a component instance of a particular type, the user's resolution method will be given the opportunity to provide the instance.

If no dependency resolution method is provided, Forms falls back to the previous behavior of creating the components as needed. If the dependency resolution method returns null for a requested component, Forms falls back to creating the components itself.

When resolving an Effect or handler (e.g., a customer renderer), specifying the resolution method is sufficient. If the user wants to resolve a service from the DependencyService, they will have to use the new Resolve<T>() method.

A brief example of using this with an IoC container

Bugs Fixed

API Changes

Xamarin.Forms.Internals.DependencyResolver (new class)

public delegate object ResolveDelegate(Type type, params object[] args);
public static void ResolveUsing(ResolveDelegate resolveDelegate)
public static void ResolveUsing(Func<Type, object> resolveFunc)

Xamarin.Forms.DependencyService

public static T Resolve<T>(DependencyFetchTarget fallbackFetchTarget = DependencyFetchTarget.GlobalInstance)

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
hartez added 2 commits Feb 7, 2018
@dansiegel

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

Will the registrar need to be registered before Forms.Init()? Or will it be safe to register the registrar inside the App in shared code?

@brianlagunas

This comment has been minimized.

Copy link

commented Feb 13, 2018

Could you clarify the usage? For example, can we assume that this would be the expected implementation.

public partial class App : Application
{
    IContainer _someContainer = new SomeContainer();
    public App()
    {
        InitializeComponent();
        Registrar.ResolveUsing(type => _someContainer.Resolve(type));
        MainPage = new MainPage();
    }
}
@brianlagunas

This comment has been minimized.

Copy link

commented Feb 13, 2018

By the way, I think the name should be reconsidered. The term Registrar is not an accurate representation of what is happening. You aren't registering anything. You are providing a delegate/Action to be used in place of Activator.CreateInstance to create/resolve objects.

I think there are more appropriate names for this class. Here are some alternatives:

  • DependencyResolver
  • FormsResolver
  • Resolver

I am personally leaning towards DependencyResolver since this more accurately represents the function of the class. It resolves objects and supplies dependencies when using DI.

Really anything other than "Registrar" would be better :)

@hartez hartez changed the title Use di container Allow users to specify resolution method for handlers, effects, and services Feb 13, 2018
@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Will the registrar need to be registered before Forms.Init()? Or will it be safe to register the registrar inside the App in shared code?

There's no explicit requirement to set the resolution method before Forms.Init. It can be set any time.

@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Could you clarify the usage?

The example you give in your comment is a valid one.

The only constraint is that the Registrar needs to know about your resolution delegate if you want the Registrar to use it. If there are services in your container which your application needs during startup, you will have to notify the Registrar of your delegate early on. If your container is providing a particular type of Effect, the Registrar will need to know about the delegate before you try to create a control using that Effect. You can set the delegate as early or as late as you want, it just has to be available to the Registrar when the Registrar needs an instance of a type.

@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Adding a DNM; I want to think about some API changes.

@dansiegel

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

There's no explicit requirement to set the resolution method before Forms.Init. It can be set any time.

@hartez thanks for the clarification, I wasn't 100% sure given the Gist... I would think that as long as you've set this prior to setting MainPage there shouldn't be a Renderer or Effect being created.

I might add, I do agree with @brianlagunas that the naming of Registrar feels a bit misleading.

@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

To be clear: I didn't choose the name Registrar; that class has been around for a long time. It just happens to be (for better or worse) the place where (most) dynamic object creation in Forms occurs. It also does other stuff that makes the name of the class make more sense.

That said, as I was responding to @brianlagunas about the name, I realized there might be an opportunity here to split Registrar into more appropriately-named parts. I'm taking a look at that right now.

@samhouts samhouts added this to Ready in v3.1.0 via automation Feb 13, 2018
@samhouts samhouts moved this from Ready to In Review in v3.1.0 Feb 13, 2018
@samhouts samhouts moved this from In Review to In Progress in v3.1.0 Feb 13, 2018
hartez added 2 commits Feb 13, 2018
Remove Reset method; simplify internal API;
@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

@dansiegel @brianlagunas I've extracted the dependency resolution / dynamic object creation bits of Registrar into a separate class called DependencyResolver. So now the injection point for your resolution delegate is DependencyResolver.ResolveUsing(), which makes a bit more sense.

@rogihee

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2018

Does this PR also enables control/configuration on whether the Registrar will scan all assemblies for all known Effect/Renderer attributes? Still curious about the performance impact of that one.

@hartez

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2018

Does this PR also enables control/configuration on whether the Registrar will scan all assemblies for all known Effect/Renderer attributes? Still curious about the performance impact of that one.

This PR doesn't address that.

I actually spent time working up a proof-of-concept on that exact feature a week or so ago. As it turns out, paring down the number of assemblies scanned for custom attributes does very little to affect startup performance. GetCustomAttributes is actually pretty fast. Even on the oldest, creakiest Android device I had available the time spent reflecting for the attributes was sub-millisecond for most assemblies. The overall performance gains of the feature ended up being noise even in a project with over 100 assemblies. (I was very disappointed.)

@rogihee

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2018

Bummer! I had high hopes for it :-). Thanks for trying it out though!

@rmarinho rmarinho requested review from jassmith and kingces95 Feb 14, 2018
@samhouts samhouts added this to In Progress in Enhancements Feb 14, 2018
@samhouts samhouts moved this from In Progress to In Review in v3.1.0 Feb 14, 2018
Copy link
Member

left a comment

Will continue review after DependencyResolver L35-46 is covered.

return Resolver?.Invoke(type, args);
}

internal static object ForceResolve(Type type, params object[] args)

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

Suggest ResolveOrActivate

This comment has been minimized.

Copy link
@hartez

hartez Feb 17, 2018

Author Member

I went with ResolveOrCreate.

{
public static class DependencyResolver
{
public delegate object ResolveDelegate(Type type, params object[] args);

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

Suggest we use Func<Type, object[], object>; Suggest we conserve the number of new Types introduced at the expense syntactic sugar at our call site; We can just put the args into a new object[] and save ourselves a bit of public surface area, no?

This comment has been minimized.

Copy link
@hartez

hartez Feb 17, 2018

Author Member

Done.

{
var result = Resolve(type, args);

if (result != null) return result;

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

Suggest we throw if Type is not assignable from the result.

This comment has been minimized.

Copy link
@hartez

hartez Feb 17, 2018

Author Member

Done.

{
result = (Effect)Activator.CreateInstance(effectType);
result = DependencyResolver.ForceResolve(effectType) as Effect;

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

Suggest considering cast instead of as; The result should either be of effectType or null, no?

This comment has been minimized.

Copy link
@hartez

hartez Feb 17, 2018

Author Member

Yep. Done.


if (result != null) return result;

if (args.Length > 0)

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

I'm not sure what case the following code handles; Suggest we add unit tests to cover 35:46.

This comment has been minimized.

@@ -0,0 +1,31 @@
<Type Name="DependencyResolver+ResolveDelegate" FullName="Xamarin.Forms.Internals.DependencyResolver+ResolveDelegate">

This comment has been minimized.

Copy link
@kingces95

kingces95 Feb 15, 2018

Member

We might consider if some dev time now drafting some isense docs might preempt questions/confusion in the forums.

hartez and others added 3 commits Feb 17, 2018
…lic delegate type;
@rmarinho rmarinho merged commit 1030013 into master Feb 26, 2018
13 checks passed
13 checks passed
VSTS: Android API19 Validation Fast Renderers UITests Finished
Details
VSTS: Android API19 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API23 Validation Fast Renderers UITests Finished
Details
VSTS: Android API23 Validation Legacy Renderers UITests Finished
Details
VSTS: Android API25 Validation Fast Renderers UITests Finished
Details
VSTS: Android API25 Validation Legacy Renderers UITests Finished
Details
VSTS: Xamarin Forms (PR Builds) Succeeded PR process
Details
VSTS: Xamarin Forms OSX PR-1870 - (1412996) succeeded
Details
VSTS: Xamarin Forms Windows VS2017 PR-1870 - (1412983) succeeded
Details
VSTS: iOS10 Validation UITests Finished
Details
VSTS: iOS11 Validation UITests Finished
Details
VSTS: iOS9 Validation UITests Finished
Details
license/cla All CLA requirements met.
Details
v3.1.0 automation moved this from In Review to Done Feb 26, 2018
Enhancements automation moved this from In Progress to Done Feb 26, 2018
jsuarezruiz added a commit to XamarinFormsCommunity/Xamarin.Forms that referenced this pull request Feb 28, 2018
* 'master' of https://github.com/xamarin/Xamarin.Forms: (23 commits)
  [C] use direct cast
  [Core, iOS, Android, UWP, WPF] Hide scroll view scroll bars (xamarin#1910)
  Allow users to specify resolution method for handlers, effects, and services (xamarin#1870) fixes xamarin#1739
  [Build] Update submodule
  Capitalization keyboard flag additions for Entry/Editor (xamarin#1683) (xamarin#1833)
  [Build] Don't specify .net sdk version
  Simplify event raising invocation pattern (xamarin#1971)
  [iOS Maps] Pin rendering customization (xamarin#1065)
  set csharp_space_after_keywords_in_control_flow_statements to true to fit our design guide lines (xamarin#1964)
  [iOS] Add shadow effect (xamarin#1896)
  Added support for ListView full width separators on iOS (xamarin#1854) fixes xamarin#1665
  Support CascadeInputTransparent to Tizen (xamarin#1916)
  [Android]?Remove UserVisibleHint (xamarin#1550) fixes xamarin#1438
  [iOS] ViewDidLayoutSubviews after removing page (xamarin#1532) fixes xamarin#1426
  Use relative URL to support recursive checkout in VSTS (xamarin#1926)
  [UITest] Fix test for UITest package update (xamarin#1923)
  Implemented MaxLength property on Entry and Editor (xamarin#1880)
  [Build]Fix master and build (xamarin#1920)
  [Build] Fix windows cert
  Fix to absolute URL
  ...

# Conflicts:
#	Xamarin.Forms.Controls/CoreGalleryPages/EditorCoreGalleryPage.cs
#	Xamarin.Forms.Controls/CoreGalleryPages/EntryCoreGalleryPage.cs
#	Xamarin.Forms.Core/InputView.cs
#	Xamarin.Forms.CustomAttributes/TestAttributes.cs
#	Xamarin.Forms.Platform.Android/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.Android/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.MacOS/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.MacOS/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.Tizen/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.Tizen/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.WPF/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.WPF/Renderers/EntryRenderer.cs
#	Xamarin.Forms.Platform.iOS/Renderers/EditorRenderer.cs
#	Xamarin.Forms.Platform.iOS/Renderers/EntryRenderer.cs
@rookiejava rookiejava referenced this pull request Mar 9, 2018
3 of 4 tasks complete
@hartez hartez deleted the use-di-container branch Mar 21, 2018
@dansiegel

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2018

hey guys can we please get this moved from vNext+1 to vNext... would really like to see this in v3.0

@jbachelor

This comment has been minimized.

Copy link

commented Mar 22, 2018

+1 for the suggestion from @dansiegel to move this up to vNext. I know my team could definitely benefit from this.

@agriffith

This comment has been minimized.

Copy link

commented Mar 22, 2018

We could use this sooner rather than later, as well.

@samhouts samhouts added the API-change label Apr 3, 2018
@samhouts samhouts added this to the 3.1.0 milestone May 5, 2018
@samhouts samhouts modified the milestone: 3.1.0 Jun 1, 2018
@samhouts samhouts removed this from Done in Enhancements Jun 12, 2018
@samhouts samhouts added the partner label Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v3.1.0
  
Done
10 participants
You can’t perform that action at this time.