parse-community / Parse-SDK-dotNET

Parse SDK for .NET, Xamarin, Unity.
http://parseplatform.org
Apache License 2.0
323 stars 260 forks source link

Finish Refactor, Make Configurable Controller Pipeline, and Make ParseClients Dependency-Injectable #329

Closed TheFanatr closed 4 years ago

TheFanatr commented 4 years ago

This does what said I would do at the end of one of my comments on #294, namely that ParseClient is now instantiable, but optionally usable as a singleton, with the ability to configure a full service pipeline for each instance, replacing the previous plugin system with a monolithic IServiceHub which contains all controllers needed for the SDK to operate. This allows the SDK operations to be fully customized, with any internal component swappable with a custom implementation, to change the way the SDK works, while also allowing use cases such as multiple concurrent users for use on servers, as well as connections to multiple Parse Server instances at once. This also theoretically makes it a lot more easy for users to fix platform-specific issues in a fairly straightforward manner by simply placing a new controller implementation into the IMutableServiceHub exposed to a IServiceHubMutator that they implement, which can be used by simply adding it to ParseClient constructor calls, and can be redistributed as one file. Using this pipeline, having the ParseClient constructor manually configure certain services which are deemed problematic based on some given types is no longer necessary, as provided IServiceHubMutator implementations can now perform such tasks on request, in a highly-customizeable fashion. The solution to make Unity and some other platforms work, namely via configuring ParseClient.Configuration and providing an IStorageConfiguration, is now replaced with components that can perform these actions separately, on demand; specifically, via the provided AbsoluteCacheLocationMutator, MetadataMutator, and/or RelativeCacheLocationMutator. This was tested with the latest beta build of Unity with the default player settings, as well as a Blazor server project with multiple clients connected concurrently, with ACL support. These demonstration projects will be released at some point in the near future. The instructions for how to use the SDK in this form are in the updated README.md.

I have also finished reorganizing the folder structure into one that makes much more sense, and will help people find files more easily. From this, the project can be modularized out much more easily, and the beginnings of an extracted abstractions library, under the Abstractions folder, can also be found. I may add the code for configuring the problematic-on-cross-platform controllers manually to a monolithic IServiceHubMutator implementation that comes with the SDK so it's easier for people to troubleshoot, but it may just be simpler for platform-specific libraries with such configuration mutators be released on NuGet, or in Unity's case as a file via the package manager.

To be clear, the part in the updated README.md under the "Use in Unity Clients" section, can be applied to any other platform where automatic value generation for metadata items via reflection, or some other bug, has caused initialization to fail on a given platform.

TheFanatr commented 4 years ago

Here are the use case demonstrations.

TheFanatr commented 4 years ago

@TobiasPott can you please test these, and review the changes? Also my apologies for the radio silence for so long.

TobiasPott commented 4 years ago

@TheFanatr Hi Alex, you're welcome and yes as soon as I find the time to give them a test I'll do so. The past weeks have shifted my focus quite a lot and most things I intended to do on this repository still wait for my attention. So no worries about your silence and I'm happy to see you're still with us =).

TheFanatr commented 4 years ago

@TobiasPott have you had a chance to look at this yet? I’ve now noticed that I made some changes to the sample projects in my other repo without testing them, so I will fix that, but the Unity sample should still work. This is a fairly significant set of changes to the current master branch, so I’d appreciate feedback on if it worked for you and what you think about the changes I’m proposing.

TobiasPott commented 4 years ago

@TheFanatr unfortunately I did not have the time yet to take a look at it but I have it on my schedule (had it for this week) but will be able to start today (if my plans don't get busted again).

I'm a bit confused what I should look at now, after you mentioned that you have fix changes in sample projects, should I take a look only on the Unity one or on the others too? I've would have checked everything I'm able to check, unless you think I should skip anything beside the Unity part and wait for your changes on the samples?

TheFanatr commented 4 years ago

@TobiasPott Try them all, but the sample I made with Blazor may not work because I removed a workaround for something and didn’t check if it still worked.

TobiasPott commented 4 years ago

@TheFanatr Okay, I did go through the Console and Unity example (I was slightly amazed that you used the UIElements for runtime stuff). I'll take a look at the commits more in depth in the next days (to have it finished at the end of the week).

One question I came up with a second ago. Did you run the Unity example on any target devices (Android, iOS, Windows), I only tried in-editor and would do it for iOS, Android and Windows Standalone.

TheFanatr commented 4 years ago

I was slightly amazed that you used the UIElements for runtime stuff

Yeah, I was going to use the built-in UI but then I remembered how much of a pain it was last time I did, and UIElements was basically perfect for my use case so all I had to do was find a renderer and figure out how to hack it to do what I want.

Did you run the Unity example on any target devices (Android, iOS, Windows), I only tried in-editor and would do it for iOS, Android and Windows Standalone.

I only tested it in the editor and a build for Windows Standalone. I have multiple iOS devices, so I could look into testing it on one of them, but I’ve never done it before and don’t have a macOS device; if I’m required to have one, I won’t be able to do it because I don’t really want to set up a whole VM just for this unless there’s no other option and it needs to be done for this PR to get merged.

TheFanatr commented 4 years ago

Also just to note, a lot of the commits make it seem like some important files were outright deleted but really their code was either just siphoned out and put somewhere else, or they were just moved. A lot of them were at least partially rewritten too.

TobiasPott commented 4 years ago

Okay, it is good to know that you did try it on windows standalone. You would need a macOS system and access to a developer account to deploy the app on iOS devices, so this would be a hassle for you to go through. I've already tried it on iOS and Android but need to redo the build process more focused to ensure I don't miss any additional settings or configurations a Unity project might need for those platforms. I'll think in the first place it will suffice when I've tried it on the mobile devices, as I'll go over it on those platforms multiple times I assume.

I'll keep it in mind to not panic when I check the commits of your PR. I assume that you did not give it a try using Xamarin for the above mentioned mobile platforms and would put that also on my list to check out.

TobiasPott commented 4 years ago

@TheFanatr I've finished looking over the commits and your changes and did not find anything you changed which seemed worth of changing. I've also finished testing the Parse SDK on the following platforms and frameworks:

For all of those platforms it is necessary to use mutators to replace the default implementation and provide valid metadata and valid caching paths. For the Unity on Android it is required to tick the an specific option to prevent Unity from stripping networking code from .NET libraries (and which seems obvious to be required for a SDK which communicates with servers ^^). It is the "Internet Access" option which needs to be set to "Require". The naming and default values may differ by Unity version and I would add this general information to the readme after merging your pull request. For Unity on iOS it is required to add a link.xml file to prevent code stripping of the Parse SDK itself (which causes the removal of loads of not directly referenced code). This increases the build time in Xcode as more .NET code is converted to C++. I'll add the information and the link.xml sample to the readme after merging the pull request.

TL;DR: I'm done with the review, will resolve the merge conflict and merge your pull request today and prepare the additional information for the above mentioned platforms.

Thank you very, very much for your work and effort and contribution to this project. =)