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

Allow developer to set a custom web socket url #111

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

FortuneN
Copy link

  • Allow developer to set a custom web socket url
  • Default behavior does not break current usages
  • Default behavior allows more usages whose URL ends with "/mirrorsharp"
  • When a WebSocketUrl is set, it is used instead of the default behaviour
  • We are lenient with the case of the URL
  • We are lenient with leading or following spaces

[ISSUE] #110

- Default behavior does not break current usages
- Default behavior allows more usages whose URL ends with "/mirrorsharp"
- When a WebSocketUrl is set, that is used
- We are lenient with the case of the URL 
- We are lenient with leading or following spaces
@FortuneN FortuneN requested a review from ashmind January 15, 2020 15:43
@ashmind
Copy link
Owner

ashmind commented Jan 18, 2020

Thanks for this! I think the goal definitely makes sense. I would be keen to investigate a bit further on the new endpoint routing -- seems like something we could use here? Not sure yet, haven't tried it myself.

Summary path change
-> the 'wss:/' part is not necessary
@ashmind
Copy link
Owner

ashmind commented Jan 22, 2020

I have investigated the endpoint routing a bit, and I think we don't have to handle the URL manually.

What we could do instead is:

  1. If endpoint middleware is not used, specific URL can be mapped as
app.Map("/mirrorsharp", app => app.UseMirrorSharp());

Obviously we want people to avoid mistakes, so I would actually obsolete UseMirrorSharp and expose MapMirrorSharp instead, but put URL as a separate parameter instead of options to match standard Map.

  1. If endpoint middleware is used, specific URL can be mapped as
    app.UseEndpoints(endpoints =>
    {
        endpoints.Map("/mirrorsharp", endpoints.CreateApplicationBuilder()
            .UseMirrorSharp()
            .Build());
    });

This is obviously too verbose, so providing another MapMirrorSharp method on endpoints would make sense (with required URL I think, for clarity).

So, in summary, I suggest the following:

  1. New MapMirrorSharp extension method on IApplicationBuilder, taking an URL (should it be a required and a first parameter, for clarity?)
  2. New MapMirrorSharp extension method on IEndpointRouteBuilder, taking a URL as a required first parameter for clarity
  3. Obsolete current UseMirrorSharp usage.

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

Successfully merging this pull request may close these issues.

2 participants