robinrodricks / FluentStorage

A polycloud .NET cloud storage abstraction layer. Provides Blob storage (AWS S3, GCP, FTP, SFTP, Azure Blob/File/Event Hub/Data Lake) and Messaging (AWS SQS, Azure Queue/ServiceBus). Supports .NET 5+ and .NET Standard 2.0+. Pure C#.
MIT License
263 stars 33 forks source link

Remove Newtonsoft.Json from FluentStorage project #43

Closed GiampaoloGabba closed 10 months ago

GiampaoloGabba commented 10 months ago

The main project FluentStorage is referencing Newtonsoft.Json without using it.

To me, its better to not take such dependency in the main project, so we can reference it in our main application layer without taking external dependencies, and then reference to desired implementation in our infrastructure project. Then if the implementation needs Newstonsoft.Json, we take the dependency here.

Also i would like to take a step further and create an abstraction project (FluentStorage.Abstractions), wich contains all the public interfaces.

So we can reference this in our application project, then bind the desired implementation with dependency injection in the infrastructure.

If you are good with this idea i can prepare another PR, otherwise i'll implement the interfaces only in my project :)

robinrodricks commented 10 months ago

The main project FluentStorage is referencing Newtonsoft.Json without using it.

Great find, accepted.

Creating of new interfaces

What is the reason for this? Testing? You want to bind some logic to the abstract interfaces and not to the concrete implementation? What is the advantage?

Also i would like to take a step further and create an abstraction project (FluentStorage.Abstractions), wich contains all the public interfaces. So we can reference this in our application project, then bind the desired implementation with dependency injection in the infrastructure.

I don't understand the benefit. Can you elaborate? Are you trying to make your own concrete implementations of these and not bind it to the FluentStorage implementations?

GiampaoloGabba commented 10 months ago

well nevermind.... Just for some obssesion i want my application project to just refer to abstractions while all the implementations are in another package. but its not worth it for everything Fluentstorage its perfectly fine to add in my application project and then i will use the sender implementation (like smtp or mailgrid ecc..) in my infrastructure project. so... please ignore my request 😄

but its my obsession to have less reference as possibile in my applicastion project wich led me to find the unusued newtonsoft.json, so at least something good happended 😄

robinrodricks commented 10 months ago

Creating of new interfaces

What is the reason for this? Testing? You want to bind some logic to the abstract interfaces and not to the concrete implementation? What is the advantage?

I'm willing to accept your PR but at least tell me one concrete advantage?

I'm asking you about the new interfaces like IBlob etc.

GiampaoloGabba commented 10 months ago

regarding the new interfaces my idea was like this comment in the old repo: https://github.com/aloneguid/storage/issues/85

Wouldn't it be a good idea to move all interfaces (which should be super stable) to an own package which is called e.g. Storage.Net.Abstraction (like Microsoft.Extensions.Logging.Abstractions) which will be super stable without any external NuGet dependencies.

This way "service" libraries only need to reference this super thin abstraction library and only the app roots (where you set up the DI system, etc.) needs to reference the actual implementations. This way you it's easier to avoid versioning problems (as most libs only depend on this abstactions lib) and the libs only reference a single abstaction lib.

There was even a rejected pull request: https://github.com/aloneguid/storage/pull/89

But in the end the original author is right, the primary library doesn't have dependency (apart from the newtonsoft.json wich isnt used anywhere).

In my proposal i was also wrong, we dont need to create interfaces like IBLob, but just move things like IBlobstorage, ITransaction, IkeyValueStorage, ecc... in another package, to have jjust the abstractions and no logic inside. But is not worth the hassle, imho. I realized it now.

Btw i just found that i made a mess... i'm working to a new version for the ServiceBus plugin (#44 ), i was pretty shure that i was in a separate branch but i was in the same of this pr... so i will close this to not make confusion and will open a new, clean, PR for the newtonsoft.json removal.

Also the changes for IBlob and IMessage where never meant to drop here. I just got some Git confusion 😄 I didnt realize that i forgot to create new branches for my experiments so everything was dropped here i didnt even noticed... 😢

Sorry for the confusion!

robinrodricks commented 10 months ago

Thanks!