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.

Sign up for GitHub

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

Jump to bottom

IHttpClientFactory returns the wrong type, making it impossible to implement a custom IHttpConnection #6647

Open
1 task done
zaneclaes opened this issue Oct 30, 2023 · 7 comments · May be fixed by #6688
Open
1 task done

IHttpClientFactory returns the wrong type, making it impossible to implement a custom IHttpConnection #6647

zaneclaes opened this issue Oct 30, 2023 · 7 comments · May be fixed by #6688
Labels
🎉 enhancement New feature or request 🌶️ strawberry shake

Comments

@zaneclaes
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Product

Strawberry Shake

Describe the bug

Presumably, the goal of exposing a DI version of IHttpConnection is to allow the end-user to plug the connection with a different implementation. In my case, System.Net.HttpClient is totally incompatible with my deployment. This should be the perfect use case for IHttpConnection, so I can plug it with a compatible implementation.

Unfortunately, even if a different connection is provided, Strawberry Shake still attempts to create its own HTTP client:

AddTransient<StrawberryShake.Transport.Http.IHttpConnection, MyHttpConnection>()

Regardless of if I use Singleton, Scoped, or Transient... the MyHttpConnection is never used. Strawberry Shake appears to load up the IHttpClientFactory instance and create a new client for each request instead.

This should be easy to fix on my side, except the IHttpClientFactory is incompatible with IHttpConnection. The Factory is obligated to return a StrawberryShake.Transport.Http.HttpConnection connection, instead of a IHttpConnection:

HttpClient CreateClient(string name);

Thus, the StrawberryShake.Transport.Http library is strongly coupled to the system's implementation of System.Net.Http.HttpClient... which is in turn coupled to System.Threading, meaning that the library cannot be used at all in environments where this is unavailable (i.e., Unity WebGL).

All it would take to fix this is to change the return type to IHttpConnection. This is already what the code generators do, coercing the HttpClient into the interface type, so it appears the current implementation is unnecessary.

Steps to reproduce

Try to use a IHttpConnection that is not derived from `

Relevant log output

No response

Additional Context?

I was able to create a very hacky solution by rewriting the generated code from StrawberryShake, but it's quite annoying since I'll need to do it every time the code is generated:

global::Microsoft.Extensions.DependencyInjection.ServiceCollectionServiceExtensions.AddSingleton<global::StrawberryShake.Transport.Http.IHttpConnection>(services, sp =>
            {
                return new MyHttpConnection();
            });

Version

13.6.1
@zaneclaes zaneclaes added the 🐛 bug Something isn't working label Oct 30, 2023
@michaelstaib
Copy link
Member

We initially designed strawberry shake as a client for blazor and nothing else :)

At some point we will do a redesign and abstract in more... at the moment our focus is on other things.

@michaelstaib michaelstaib added 🎉 enhancement New feature or request and removed 🐛 bug Something isn't working labels Oct 30, 2023
@michaelstaib
Copy link
Member

But you can go ahead and do a PR if you like.

@zaneclaes
Copy link
Author

@michaelstaib are you aware of anyone working on a Unity-compatible version, aside from #4066? I have successfully implemented an IHttpConnection which leverages the Unity HTTP client under the hood. However, 90% of the code is copy-pasta from the internal and sealed classes in the Transport.Http package. With a few small tweaks to Transport.Http, I could submit a PR that also allowed me to implement the Unity HTTP layer "correctly." Thus I should then be able to publish a minimal StrawberryShake.Transport.Unity package, with your approval, so that other game devs can benefit.

@michaelstaib
Copy link
Member

As I said, we are fully open to this ... do a PR and we will help you get it in.

@zaneclaes zaneclaes linked a pull request Nov 10, 2023 that will close this issue
Draft
@Vingator
Copy link

Vingator commented Apr 4, 2024

@zaneclaes did you happen to find a solution for it? I see that your PR is still in draft.
Also, what are the symptoms you had in WebGL? I currently get this message:
Unable to open DLL! Dynamic linking is not supported in WebAssembly builds due to limitations to performance and code size. Please statically link in the needed libraries.

But I'm not sure if what you described is the actual cause of my runtime error.

@zaneclaes
Copy link
Author

@Vingator full support for Unity is MUCH trickier than just this piece, but yes, I have it working in our project, including WebGL. Your error is a runtime problem, and unrelated to this thread. Specifically, you need to statically link all the necessary HotChocolate/StrawberryShake runtime DLLs compiled for the netstandard2.x framework (and their dependencies, of course). I would recommend making sure your solution works in the editor first, and then moving on to solving the run-time linking errors on the various build-targets.

@Vingator
Copy link

Vingator commented Apr 5, 2024 •
edited

@zaneclaes thanks for the quick answer! I have the app working in the editor and even on iOS/Android, but I get this exception on WebGL at the point I'm querying data, that's why I supposed it was the same issue you were facing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🎉 enhancement New feature or request 🌶️ strawberry shake
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add IHttpConnectionFactory for non-HttpClient support zaneclaes/hotchocolate
3 participants
@zaneclaes @michaelstaib @Vingator