Pass options to HubConnectionContext #11382
Conversation
BrennanConroy
requested review from
analogrelay,
halter73 and
mikaelm12
as code owners
| /// <param name="connectionContext">The underlying <see cref="ConnectionContext"/>.</param> | ||
| /// <param name="keepAliveInterval">The keep alive interval. If no messages are sent by the server in this interval, a Ping message will be sent.</param> | ||
| /// <param name="loggerFactory">The logger factory.</param> | ||
| public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory) |
There was a problem hiding this comment.
Question: Should we keep the 2 old constructors so this isn't a breaking change?
| /// <param name="streamBufferCapacity">The buffer size for client upload streams</param> | ||
| public HubConnectionContext(ConnectionContext connectionContext, TimeSpan keepAliveInterval, ILoggerFactory loggerFactory, TimeSpan clientTimeoutInterval, int streamBufferCapacity) | ||
| /// <param name="hubOptions">The options to configure the connection.</param> | ||
| public HubConnectionContext(ConnectionContext connectionContext, ILoggerFactory loggerFactory, HubOptions hubOptions) |
There was a problem hiding this comment.
Make sure we announce this breaking change.
There was a problem hiding this comment.
Should the options come before the logger? Then we could make the logger an optional param 😄
| public HubConnectionContext(Microsoft.AspNetCore.Connections.ConnectionContext connectionContext, System.TimeSpan keepAliveInterval, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { } | ||
| public HubConnectionContext(Microsoft.AspNetCore.Connections.ConnectionContext connectionContext, System.TimeSpan keepAliveInterval, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, System.TimeSpan clientTimeoutInterval) { } | ||
| public HubConnectionContext(Microsoft.AspNetCore.Connections.ConnectionContext connectionContext, System.TimeSpan keepAliveInterval, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory, System.TimeSpan clientTimeoutInterval, int streamBufferCapacity) { } | ||
| public HubConnectionContext(Microsoft.AspNetCore.Connections.ConnectionContext connectionContext, Microsoft.AspNetCore.SignalR.HubOptions hubOptions, Microsoft.Extensions.Logging.ILoggerFactory loggerFactory) { } |
There was a problem hiding this comment.
Do we have a standard order for parameters? I.e. do we always try to keep logger parameters last and options parameters second to last?
There was a problem hiding this comment.
Should this context be the hub options? Why isn't it its own context
There was a problem hiding this comment.
I chose HubOption because it contains the settings we want to pass to HubConnectionContext and I didn't want to make a new public type. We can change that if you'd like.
There was a problem hiding this comment.
Yes I think we should have a HubConnectionContextOptions as this will start to break down the minute we need to pass something that isn't exactly an option.
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
ghost
deleted the
Fixes #11258
Reusing
HubOptionto pass values toHubConnectionContext.