Skip to content

[API] Add client result support to Java client #41777

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

Closed
Tracked by #5280
BrennanConroy opened this issue May 20, 2022 · 6 comments
Closed
Tracked by #5280

[API] Add client result support to Java client #41777

BrennanConroy opened this issue May 20, 2022 · 6 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation feature-client-java Related to the SignalR Java client
Milestone

Comments

@BrennanConroy
Copy link
Member

BrennanConroy commented May 20, 2022

Background and Motivation

Adding async .On handlers for the SignalR Java client in order to better support client results.
Adding .On overloads (and types) to support client results.

Proposed API

public interface Action1<T1> {
    void invoke(T1 param1);
}

+ public interface Action1Completable<T1> {
+     Completable invoke(T1 param1);
+ }

//...

+ Action8Completable<T1, ..., T8>
+ public interface Function<T> {
+     T invoke();
+ }

+ public interface FunctionSingle<T> {
+     Single<T> invoke();
+ }

+ public interface Function1<T1, T> {
+     T invoke(T1 param1);
+ }

+ public interface Function1Single<T1, T> {
+     Single<T> invoke(T1 param1);
+ }

// Up to 8
class HubConnection {
+    public <T> Subscription on(String target, Function<T> callback);

+    public <T> Subscription on(String target, FunctionSingle<T> callback);

+    public <T1, T> Subscription on(String target, Function1<T1, T> callback, Class<T1> param1);

+    public <T1> Subscription on(String target, Action1Completable<T1> callback, Class<T1> param1);

// etc. to 8 args each
}

Usage

hubConnection.on("Send", () -> {
    return Single.just(10);
});

hubConnection.on("Send", () -> {
    return 10;
});

hubConnection.on("Send", () -> {
    return Completable.Complete();
});
@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers feature-client-java Related to the SignalR Java client api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 20, 2022
@ghost
Copy link

ghost commented May 20, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented May 23, 2022

API review notes:

  1. What about the breaking change?
    // Ambiguous between Action<String> and Function<String, Object>
    On(param ->
    {
      throw new RuntimeException();
    }, String.class);
    We think this is rare enough that we can document it. It's also easy to work around by specifying a type. If we wanted to remove the break, we could Class<TResult> argument but this seems annoying.
  2. Can we come up better name than T?
    TResult
  3. Can we get rid of FunctionSingle and check after the callback returns if the returned object is a Single?
    Maybe. It wouldn't be obvious Single is supported though, so we shouldn't do it.
  4. Is Function too generic of a name?
    Maybe, but we've already shipped Action, so this shouldn't be any worse.
package com.microsoft.signalr;

+ public interface ActionCompletable {
+     Completable invoke();
+ }
+ public interface Action1Completable<T1> {
+     Completable invoke(T1 param1);
+ }

// Up to 8
+ Action8Completable<T1, ..., T8>
package com.microsoft.signalr;

+ public interface Function<TResult> {
+     T invoke();
+ }
+ public interface FunctionSingle<TResult> {
+     Single<TResult> invoke();
+ }
+ public interface Function1<T1, TResult> {
+     TResult invoke(T1 param1);
+ }
+ public interface Function1Single<T1, TResult> {
+     Single<TResult> invoke(T1 param1);
+ }
// Up to 8
+ Function8Single<T1, ..., T8, TResult>
package com.microsoft.signalr;

class HubConnection {
+    public Subscription on(String target, ActionCompletable callback);
+    public <TResult> Subscription on(String target, Function<TResult> callback);
+    public <TResult> Subscription on(String target, FunctionSingle<TResult> callback);

+    public <T1> Subscription on(String target, Action1Completable<T1> callback, Class<T1> param1);
+    public <T1, TResult> Subscription on(String target, Function1<T1, TResult> callback, Class<T1> param1);
+    public <T1, TResult> Subscription on(String target, Function1Single<T1, TResult> callback, Class<T1> param1);
// Up to 8
+    public <T1, ..., T8> Subscription on(String target, Action8Completable<T1, ..., T8> callback, Class<T1> param1, ..., Class<T8> param8);
+    public <T1, ..., T8, TResult> Subscription on(String target, Function8<T1, ..., T8, TResult> callback, Class<T1> param1, ..., Class<T8> param8);
+    public <T1, ..., T8, TResult> Subscription on(String target, FunctionSingle<T1, ..., T8, TResult> callback, Class<T1> param1, ..., Class<T8> param8);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 23, 2022
@BrennanConroy BrennanConroy added this to the 7.0-preview6 milestone May 24, 2022
@BrennanConroy BrennanConroy self-assigned this May 24, 2022
@BrennanConroy
Copy link
Member Author

Because of how generics work in Java
The new overloads we are trying to add for ActionCompletable and FunctionXSingle make the .on method calls ambiguous.

public <T1, TResult> Subscription on(String target, Function1<T1, TResult> callback, Class<T1> param1);
public <T1, TResult> Subscription on(String target, Function1Single<T1, TResult> callback, Class<T1> param1);
public <T1> Subscription on(String target, Action1Completable<T1> callback, Class<T1> param1);

The above is ambiguous.

After discussion with Stephen, we think it would be best to rename the result returning .on methods and to only add the async versions, no sync version.

New proposal is to add onWithResult methods.

package com.microsoft.signalr;

+ public interface ActionCompletable {
+     Completable invoke();
+ }
+ public interface Action1Completable<T1> {
+     Completable invoke(T1 param1);
+ }

// Up to 8
+ Action8Completable<T1, ..., T8>

+ public interface FunctionSingle<TResult> {
+     Single<TResult> invoke();
+ }
+ public interface Function1Single<T1, TResult> {
+     Single<TResult> invoke(T1 param1);
+ }
// Up to 8
+ Function8Single<T1, ..., T8, TResult>

class HubConnection {
+    public Subscription on(String target, ActionCompletable callback);
+    public <TResult> Subscription onWithResult(String target, FunctionSingle<TResult> callback);

+    public <T1> Subscription on(String target, Action1Completable<T1> callback, Class<T1> param1);
+    public <T1, TResult> Subscription onWithResult(String target, Function1Single<T1, TResult> callback, Class<T1> param1);
// Up to 8
+    public <T1, ..., T8> Subscription on(String target, Action8Completable<T1, ..., T8> callback, Class<T1> param1, ..., Class<T8> param8);
+    public <T1, ..., T8, TResult> Subscription onWithResult(String target, FunctionSingle<T1, ..., T8, TResult> callback, Class<T1> param1, ..., Class<T8> param8);
}

Alternatives would be to add an extra parameter to the new overloads which would basically do nothing except make it obvious to Java which overload to use.

+    public <T1, TResult> Subscription on(String target, Function1Single<T1, TResult> callback, Class<T1> param1, bool hasResult);

@BrennanConroy BrennanConroy added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Aug 5, 2022
@ghost
Copy link

ghost commented Aug 5, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@halter73
Copy link
Member

halter73 commented Aug 8, 2022

API Review Notes:

  • Can we make callback an object and just reflection it up?
    • Maybe, but the docs would be terrible.
  • What about HubConnection.results.on or HubConnection.clientResults.on?
    • It's weird to expose a field in Java and we don't want to do getResults().on() lol.
  • Why do we need async callbacks for things that do not return?
    • It might be nice, but it's not necessary. We can add later.

API Approved!

package com.microsoft.signalr;

+ public interface FunctionSingle<TResult> {
+     Single<TResult> invoke();
+ }
+ public interface Function1Single<T1, TResult> {
+     Single<TResult> invoke(T1 param1);
+ }
// Up to 8
+ Function8Single<T1, ..., T8, TResult>

class HubConnection {
+    public <TResult> Subscription onWithResult(String target, FunctionSingle<TResult> callback);

+    public <T1, TResult> Subscription onWithResult(String target, Function1Single<T1, TResult> callback, Class<T1> param1);
// Up to 8
+    public <T1, ..., T8, TResult> Subscription onWithResult(String target, FunctionSingle<T1, ..., T8, TResult> callback, Class<T1> param1, ..., Class<T8> param8);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 8, 2022
@BrennanConroy
Copy link
Member Author

Done via #41774

@BrennanConroy BrennanConroy added the Docs This issue tracks updating documentation label Aug 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-signalr Includes: SignalR clients and servers Docs This issue tracks updating documentation feature-client-java Related to the SignalR Java client
Projects
None yet
Development

No branches or pull requests

3 participants