search

twitchax / AspNetCore.Proxy

ASP.NET Core Proxies made easy.
MIT License
525 stars 83 forks source link

Added support to configure the websocket options. #34

Closed havarnov closed 4 years ago

havarnov commented 4 years ago

Didn't find a good way to test this, but if you have any ideas I'll gladly add a test.

twitchax commented 4 years ago

Hi @havarnov, so, I actually implemented this already for the new release.

Check it out in this branch, and let me know what you think.

https://github.com/twitchax/AspNetCore.Proxy/tree/4-release

havarnov commented 4 years ago

Hi @havarnov, so, I actually implemented this already for the new release.

Check it out in this branch, and let me know what you think.

https://github.com/twitchax/AspNetCore.Proxy/tree/4-release

Aha, I didn't realize. Then I guess you can discard this PR.

I have tested the 4-release branch and it works for my use case. I'm using the ControllerBase.ProzyAsync method.

I do have one suggestion on those methods, what about using the following pattern?

public static Task ProxyAsync(this ControllerBase controller, string httpEndpoint, string wsEndpoint, Action<HttpProxyOptions> httpProxyOptionsBuilder = null, Action<WsProxyOptions> wsProxyOptionsBuilder = null);
twitchax commented 4 years ago

@havarnov, I had thought about doing that.

The problem is that it encourages bad behavior. The builder methods are great for the Startup methods because I can "build" the resulting routes on your behalf on startup once.

The problem with using the builders with the ProxyAsync extension method is that that method is run every invocation, and the builders would get "built" every invocation. In general, this is not terrible, but it is a bad habit to get into, when the options are likely static for every invocation. If they aren't, then you can still "build" the options earlier in the invocation to your controller endpoint.

Does that make sense?

twitchax commented 4 years ago

Closing in favor of 4.0.0 release.