search

intuit / QuickBooks-V3-DotNET-SDK

.Net SDK for QuickBooks REST API v3 services
Apache License 2.0
108 stars 140 forks source link

Dependency additions #108

Closed southernskip closed 5 years ago

southernskip commented 5 years ago

We use the SDK with .NET 4.8 in our application.

Seems like in the last release 9.6.1 vs 8.1 we have gone from 1 dependency (JSON.NET) to a huge list of 19 dependencies via the use of SeriLog. Given our goal of keeping our releases clean with a minimum number of third party assemblies. will this be the new normal going forward?

nimisha84 commented 5 years ago

HI @southernskip,

We wanted to support loggers for OAuth2Client and the old SDK classes too based on the feedback we had received from many devs. We want to continue the support for Serilog dependencies going forward too. Let me know if you know of any alternatives which can improve this.

Thanks, Nimisha

MatthewSteeples commented 4 years ago

We've just updated from v8.1.0.0 to v10.0.0.3 and as part of the update this pulled in 32 (!!!) extra dependencies, including a whole new database library, another JSON parsing library.

Having had a look through them, a lot of them are unnecessary and can be trimmed. For example, all of the Serilog.Sinks.* packages don't need to be included or referenced, because they're something that users of the library can install if they want to configure a particular Sink.

AdvancedLogging.Log is publicly assignable, so the configuration is best taken care of not in the main library. If we don't want to rely on the developers setting up their own logging then it can still be configured in a satellite assembly.

nimisha84 commented 4 years ago

You are right on some aspects of it but suggesting what devs can install vs what we provide them out of the box with the package was a trade off. You can still choose to remove the unwanted sinks. At this point, we are not able to prioritize this request but if you are open to doing a pull without breaking the current experience, I would be happy to review and merge. As always, please continue to give us feedback to make things better.

MatthewSteeples commented 4 years ago

@nimisha84 Thanks for the response. I've been working on making a pull request that (mostly) doesn't break anything (as the CosmosDb library is the biggest culprit, and isn't currently wired in). My main problem is that I'm having trouble generating the NuGet packages to test it. Do you have some form of script that you run as part of the release process?

nimisha84 commented 4 years ago

Oh, ok. When you build project finally in release mode, this https://github.com/intuit/QuickBooks-V3-DotNET-SDK/tree/master/IPPDotNetDevKitCSV3/Code/Intuit.Ipp.Nupkg package will create the nuget package file for you under and artifacts folder https://github.com/intuit/QuickBooks-V3-DotNET-SDK/tree/master/IPPDotNetDevKitCSV3/Code. You can review and modify it. Once we review your merge request, we can push it to our org.

MatthewSteeples commented 4 years ago

Ah thanks for that! I was building in Debug mode and it was failing telling me it couldn't find a reference. Didn't think to try Release mode.

dahlbyk commented 2 years ago

It's hard to express how surprising it is to find an API SDK has taken a dependency on a full logging framework. I strongly recommend dropping the Serilog dependency altogether in favor of the standard https://www.nuget.org/packages/Microsoft.Extensions.Logging.Abstractions/ or https://www.nuget.org/packages/Microsoft.Extensions.Logging/.

RaysceneNS commented 2 years ago

@nimisha84 This library needs to have all dependencies on SeriLog removed. As an SDK it is not its job to control how I setup logging in my application. @dahlbyk has provided some excellent suggestions as well as a PR on this topic, I too suggest that the standard Microsoft logging extensions ILogger interface is used instead of Serilog, that way consumers of this API can control how logs are persisted.

clement911 commented 2 years ago

https://github.com/better-reports/QuickBooksSharp is a alternative client that is more modern and clean.

southernskip commented 2 years ago

@nimisha84 fully agree with @RaysceneNS. App sizes are a big deal and half of these added dependencies have no relevance to the application for the majority of us who do not use serilog. Why enforce it instead of a an optional add in or generic logging interface

Shane32 commented 2 months ago

I also continue to use version 8.x because of the Serilog dependencies added in newer versions.

As stated by others:

  1. If this package were to depend on Serilog, only the minimal amount of dependencies should be referenced; for instance, the sinks should not be referenced. Each additional dependency slows build and deployment time and adds additional dependencies for security reviews and etc. Each additional dependency also increases the risk that client applications may use an incompatible version of the same dependency in their application.

  2. If this package were to depend on Serilog, no Serilog configuration should be included; the application should configure Serilog configuration, not the library.

  3. Extracting an interface for logging would be a great alternative, with additional packages that tie the main package to Serilog or other logging providers.

Also:

  1. Another alternative would be to use Microsoft.Extensions.Logging.Abstractions, which contains an extremely minimal set of interfaces in order to provide logging services to logging providers, with a bare minimum of nested dependencies. Microsoft has extremely rigid requirements for security and backwards compatibility versus a third party framework such as Serilog. I would consider referencing Microsoft.Extensions.Logging.Abstractions v2.1.1 which has zero dependencies and should be forwards compatible with all newer versions (tests could be configured to verify this). Note that version 2.1.1 is still implicitly supported by Microsoft according to this due to it being a dependency of Microsoft.AspNetCore 2.1.x: