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

Remove Newtonsoft.Json from FluentStorage project #43

Closed
wants to merge 5 commits into from

Conversation

GiampaoloGabba
Copy link
Contributor

@GiampaoloGabba GiampaoloGabba commented Oct 26, 2023

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

robinrodricks commented Oct 27, 2023

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
Copy link
Contributor Author

GiampaoloGabba commented Oct 27, 2023

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 added a commit that referenced this pull request Oct 28, 2023
Remove Newtonsoft.Json from FluentStorage project (supersedes #43)
@robinrodricks
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants