Navigation Menu

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

Breaking Changes Moving to Async #35

Closed
aarondcoleman opened this issue Jul 11, 2014 · 13 comments
Closed

Breaking Changes Moving to Async #35

aarondcoleman opened this issue Jul 11, 2014 · 13 comments

Comments

@aarondcoleman
Copy link
Owner

Here's the deal -- this is an HTTP plumbing library and .NET introduced the "async" modifier since to allow for clever thread repurposing while things like HTTP and / IO are waiting for responses. Microsoft and the .NET OSS community have also made great progress in Portable Class Libraries (PCL) which are common denominator projects that can span all the subset .NET frameworks (Windows Phone 8, Xamarin, Silverlight, etc). Not everyone is down with async yet either in the .NET community, there's a slight learning curve.

If I were to start this library over again from scratch I'd

  1. Not use restsharp as it isn't PCL compatible (but a small stalled branch is) and doesn't use the async modifier (though it has an older style async callback structure).

  2. Not consume the XML endpoints from Fitbit as the JSON ones seem to get a little more love from their API team and on several occasions malformed XML with debug data has caused issues. Plus, everyone loves JSON.net and serialization/deserialization speed.

We have some ideas on how to start to move to this, but it likely means breaking changes or a non PCL. I for one would like to start using async versions of parts of the client library right away in parts of my project that are most frequently called (GetUserProfile, TimeSeries, and DailyActivity) but cannot in one swoop replace all the methods.

So, I'm open to ideas. We might continue a long branch offshoot so we can make progress in both, we might merge the projects with a common project where sync and async/pcl live, and we might just say screw it and start an entirely new project.

Myself and @WestDiscGolf have already started exploring technically how to do this. See:

Chime in with feedback.
--Aaron

@WestDiscGolf
Copy link
Contributor

I've been letting this mull over at the back of my mind after discussing with Aaron on Friday and I think there are a couple of possible ways to progress both the current client implementation and create a new pcl/async variety at the same time.

  1. Start from scratch - This is quite a bold movement and would be interested in hearing thoughts on this.
  2. Convert the current Fitbit Client implementation into a PCL and add Async methods and remove the Sync methods - this is an all or nothing style migration which isn't ideal for existing applications which use the client.
  3. Add a new PCL to the solution for the client, move the model classes into common project which can be referenced by both client implementation projects and allow a piecemeal implementation of the newer client async methods. Possible solution layout:

image

Now naming is important and namespace clashes need to be avoided so ...

  • Current FitbitClient would stay in "Fitbit.Api" - eg. Fitbit.Api.FitbitClient
  • New FitbitClient would be in "Fitbit.Api.Portable" - Fitbit.Api.Portable.FitbitClient
  • Existing Models would stay in "Fitbit.Models"

I'm not sure how this would work with nuget though; would it turn into 3 packages - Common, Sync and Async/Portable? With the client implementations having a dependency on the Common package and either Restsharp or AsyncOAuth depending on which is selected? And Json.Net would be added in there somewhere as well but as Aaron says "everyone loves Json.Net" :-)

Thoughts?

@WestDiscGolf
Copy link
Contributor

I've checked in a starting point with the Async authenticator implementation I had in my other branch and the JsonDotNetSerializer wrapper which I had as well. I've moved over a small number of items to get things working. The serialization tests still need updating to work with the JSON representation as well. I will try and get the UserProfile Async implementation and the Handle response version in the Portable FitbitClient in the next day or so.

See : https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

Let me know your thoughts.

@WestDiscGolf
Copy link
Contributor

I've pushed the initial UserProfile implementation; not fully tested yet though but take a look.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

UserProfile selection, JSON style, tested and added to the MVC example.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

New MVC Sample project added for the new async/portable and the original MVC Sample project reverted back to using the original v1 synchronous/Restsharp based client implementation.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

Migrated Devices, changed the test file extensions to .json for the portable tests, updated the JsonDotNetSerializer wrapper, and some additional general improvement tweaks.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

Migrated Friends, added custom serializer extension for strange json representation which doesn't fit into default JSON.net parsing, tidied up the MVC controller for the sample application.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

Added initial unit tests for calling the GetDevicesAsync while faking the http calls through the client as I believe that the serialization but also the call logic needs to be tested.

In addition to this I have updated how the responses are handled as trying to throw/catch/handle exceptions in async code is not possible (and a big no no) so updated the responses from the calls to be a FitbitResponse - not 100% happy with the response definition; the data, errors and response status is fine but I think the retry value needs to be tweaked somehow but will have a think.

Let me know what you think.

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

A lot of things have moved on since I last posted; recently another few methods have been migrated and there is over 90% unit test coverage of the new portable library now.

Let me know what you think - https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

Next up; body weight, activities, sleep etc.

@WestDiscGolf
Copy link
Contributor

SetGoals method and the deprecated calls from V1 set to obsolete. Unit tests to come ...

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

All the methods have now been migrated to V2 :-)

@WestDiscGolf
Copy link
Contributor

Updated the extension method ToFullUrl to not be extension on System.String anymore after design decision. This was decided as it was on all strings in the library and wasn't appropriate for most use cases so the "extensionability" of the method was removed.

Incorrect post earlier about all migrations happened ... subscriptions still need to be done ... ooops, sorry :-)

https://github.com/WestDiscGolf/Fitbit.NET/tree/async-portable

@WestDiscGolf
Copy link
Contributor

Changed the way to mock the handlers for unit testing the new FitbitClient, updated/added some further testings including the SetGoalsAsync tests and merged/migrated the Water PR from November (#39).

Latest - https://github.com/WestDiscGolf/Fitbit.NET/

@aarondcoleman
Copy link
Owner Author

Closing this as we've already established the breaking changes are imminent.

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

No branches or pull requests

2 participants