search

matomo-org / piwik-dotnet-tracker

C# API client SDK for the Piwik Tracking API
BSD 3-Clause "New" or "Revised" License
74 stars 47 forks source link

Use HttpClient and change to async method #54

Open huan086 opened 7 years ago

huan086 commented 7 years ago

Upgrade to .NET 4.6.1. Fixes #34. Use HttpClient for requests. Change all tracking methods to async. Fixes #39.

Warning: this pull request removes all the sync methods

ptr1120 commented 7 years ago

Hi huan086,

you PR looks very good. One problem I see is the requirement for .net 4.6.1 since this would break the usage of the tracker for many people using .Net 4.5. Is there any reason why you upgraded to 4.6.1? I think async support was introduced in .Net 4.5.. Making the HttpClient as a constructor parameter is a good idea for people who are using the HttpClient as singleton in their application, would it be possible to make it as a optional parameter?

Best regards, Peter

huan086 commented 7 years ago

For the .NET 4.6.1, I re-read the deprecation notice for .NET 4.5 and realized I had a misunderstanding. Misunderstand that we shouldn't target .NET 4.5. But the advice was to upgrade the installed framework. Changing the target now.

julienmoumne commented 7 years ago

Could you elaborate why we shouldn't target .NET 4.5? And why should we update the .NET version?

I don't have much knowledge of the .NET eco system, I am trying to catch-up with you guys. If you have a link to an article with must know facts, please share it here.

Also, is there a way to know how many users won't be able to use the library any more if we update the version? Is there some public per version usage percentages so we can base the decision on some more or less concrete facts?

huan086 commented 7 years ago

Changed to target .NET 4.5. Added constructor without HttpClient parameter.

julienmoumne commented 7 years ago

Thanks for this PR.

I have to take some time to understand the implication of updating the .NET version.

If you have some helpful resources, do share.

huan086 commented 7 years ago

Probably see this? https://en.wikipedia.org/wiki/.NET_Framework#Release_history

If anyone is building desktop app, the installer for the app can skip installing .NET Framework 4.5 when on Windows 8 and above. When installing in Windows 7, installation would be longer.

Let's ignore Vista and previous version since they are no longer supported by MS

julienmoumne commented 7 years ago

We will release v3 before merging this PR.

This will allow users that may be stuck with .NET v4.0 on the production server to benefit from the recent bug fixes and the deprecation of the GPL license.

huan086 commented 7 years ago

Since this is a breaking change, it would be ideal if #25 (another breaking change) could be addressed before v4 RTM. However, I have no experience in UWP and .NET Standard. Hopefully someone experienced would help

huan086 commented 7 years ago

I realized I haven't added CancelationToken prameter to the async methods. Drawback of that is that the caller won't be able to cancel the async task, i.e. abort the HttpClient.SendAsync before it's complete. Will probably do another pull request on top of this

julienmoumne commented 7 years ago

Thank you for taking the time to educate me on the async support in .NET

I will perform a last review next week and merge your PR.

It would indeed be good to have the possibility to cancel the task. I don't know how much effort it would require though.

Concerning v4, I feel it's OK to release it with only the async support. This is except if the API changes introduced for UWP would require a developer two substantially modify the code when going from v4 to v5.

julienmoumne commented 7 years ago

Thank you

I have created https://github.com/piwik/piwik-dotnet-tracker/issues/66 which I will address to ease the process of refactoring sample code.

MERamadan commented 5 years ago

Any updates about this PR? I think updating the framework is very important to get the security updates. In addition to, .NET 4.6.1 is compatible with .NET Standard projects. The current version shows the following warning in Visual Studio

[Warning][NU1701] Package 'Piwik.Tracker 3.0.0' was restored using '.NETFramework,Version=v4.6.1' instead of the project target framework '.NETStandard,Version=v2.0'. This package may not be fully compatible with your project.