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

Add a strongly typed ServerlessHub #267

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

Y-Sindo
Copy link
Member

@Y-Sindo Y-Sindo commented Sep 2, 2021

No description provided.

Copy link

@lfshr lfshr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up my slack here! Have a couple of comments.

@@ -14,53 +14,60 @@

namespace FunctionApp
{
public class SimpleChat : ServerlessHub
public class SimpleChat : ServerlessHub<SimpleChat.IChatClient>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it not be better to copy & paste the bidirectional-chat example, and highlight the fact that it's a strongly typed example? (eg. strongly-typed-bidirectional-chat)

{
private const string NewMessageTarget = "newMessage";
private const string NewConnectionTarget = "newConnection";

public interface IChatClient
{
public Task newConnection(NewConnection newConnection);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably some discussion here around casing. The example was pascal cased.

Personally I'd want to stick to the language standards for examples.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @lfshr should be pascal

Comment on lines +70 to +73
private Task<ServiceHubContext<T>> GetAsync<THub, T>() where THub : ServerlessHub<T> where T : class
{
return _strongTypedHubServiceProvider.GetRequiredService<ServerlessHubContext<THub, T>>().HubContextTask;
}
Copy link

@lfshr lfshr Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this signature for GetAsync. As it stands though it's currently never used.

Comment on lines +84 to +91
public dynamic GetAsync(Type THubType, Type TType)
{
var genericType = typeof(ServerlessHubContext<,>);
Type[] typeArgs = { THubType, TType };
var serverlessHubContextType = genericType.MakeGenericType(typeArgs);
dynamic serverlessHubContext = _strongTypedHubServiceProvider.GetRequiredService(serverlessHubContextType);
return serverlessHubContext.HubContextTask.GetAwaiter().GetResult();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be dynamic? We seem to know all the types at compile-time.

Comment on lines +11 to +12
<ProjectReference Include="C:\Users\zityang\source\repos\azure-signalr\src\Microsoft.Azure.SignalR.Management\Microsoft.Azure.SignalR.Management.csproj" />
<ProjectReference Include="C:\Users\zityang\source\repos\azure-signalr\src\Microsoft.Azure.SignalR.Common\Microsoft.Azure.SignalR.Common.csproj" />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget these!


namespace Microsoft.Azure.WebJobs.Extensions.SignalRService
{
internal interface IInternalServiceHubContextStore : IServiceHubContextStore
{
AccessKey[] AccessKeys { get; }

public dynamic GetAsync(Type THubType, Type TType);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite understanding the necessity for this to be separated from IServiceHubContextStore.GetAsync(string hubName). Is this worth documenting?

{
var claims = GetClaims(req.Headers["Authorization"]);
return NegotiateAsync(new NegotiationOptions
var result = await NegotiateAsync(new NegotiationOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NegotiateAsync method returns ValueTask for strongly-typed hub.

@lfshr
Copy link

lfshr commented Sep 25, 2021

Linking #131

return true;
}
var baseType = type.BaseType;
while (baseType != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type.IsSubclassOf(typeof(ServerlessHub<>)) doesn't work?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type.IsSubclassOf(typeof(ServerlessHub<>)) doesn't work?

Doesn't work on an open generic I'm afraid!

@Y-Sindo
Copy link
Member Author

Y-Sindo commented Nov 2, 2021

Hi @lfshr @phantomcosmonaut This project has been merged into a mono repo, please go here for the update of the pr: Azure/azure-sdk-for-net#25075. And the repo will be archived in the future.

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.

3 participants