-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix #8974 by supporting strings in configureLogging for SignalR JS client #9252
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
Changes from all commits
238585d
9ef4b68
557cbee
406c054
6ac8a5c
eb930b6
53b5475
09038c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,31 @@ import { JsonHubProtocol } from "./JsonHubProtocol"; | |
import { NullLogger } from "./Loggers"; | ||
import { Arg, ConsoleLogger } from "./Utils"; | ||
|
||
// tslint:disable:object-literal-sort-keys | ||
const LogLevelNameMapping = { | ||
trace: LogLevel.Trace, | ||
debug: LogLevel.Debug, | ||
info: LogLevel.Information, | ||
information: LogLevel.Information, | ||
warn: LogLevel.Warning, | ||
warning: LogLevel.Warning, | ||
halter73 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
error: LogLevel.Error, | ||
critical: LogLevel.Critical, | ||
none: LogLevel.None, | ||
}; | ||
|
||
function parseLogLevel(name: string): LogLevel { | ||
// Case-insensitive matching via lower-casing | ||
// Yes, I know case-folding is a complicated problem in Unicode, but we only support | ||
// the ASCII strings defined in LogLevelNameMapping anyway, so it's fine -anurse. | ||
const mapping = LogLevelNameMapping[name.toLowerCase()]; | ||
if (typeof mapping !== "undefined") { | ||
return mapping; | ||
} else { | ||
throw new Error(`Unknown log level: ${name}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Do we want to preserve the casing on the user input here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does preserve that casing. I think that's the appropriate behavior here, since it's echoing what the user put in. |
||
} | ||
} | ||
|
||
/** A builder for configuring {@link @aspnet/signalr.HubConnection} instances. */ | ||
export class HubConnectionBuilder { | ||
/** @internal */ | ||
|
@@ -44,15 +69,26 @@ export class HubConnectionBuilder { | |
|
||
/** Configures custom logging for the {@link @aspnet/signalr.HubConnection}. | ||
* | ||
* @param {LogLevel | ILogger} logging An object implementing the {@link @aspnet/signalr.ILogger} interface or {@link @aspnet/signalr.LogLevel}. | ||
* @param {string} logLevel A string representing a LogLevel setting a minimum level of messages to log. | ||
* See {@link https://docs.microsoft.com/en-us/aspnet/core/signalr/configuration#configure-logging|the documentation for client logging configuration} for more details. | ||
*/ | ||
public configureLogging(logLevel: string): HubConnectionBuilder; | ||
|
||
/** Configures custom logging for the {@link @aspnet/signalr.HubConnection}. | ||
* | ||
* @param {LogLevel | string | ILogger} logging A {@link @aspnet/signalr.LogLevel}, a string representing a LogLevel, or an object implementing the {@link @aspnet/signalr.ILogger} interface. | ||
* See {@link https://docs.microsoft.com/en-us/aspnet/core/signalr/configuration#configure-logging|the documentation for client logging configuration} for more details. | ||
* @returns The {@link @aspnet/signalr.HubConnectionBuilder} instance, for chaining. | ||
*/ | ||
public configureLogging(logging: LogLevel | ILogger): HubConnectionBuilder; | ||
public configureLogging(logging: LogLevel | ILogger): HubConnectionBuilder { | ||
public configureLogging(logging: LogLevel | string | ILogger): HubConnectionBuilder; | ||
public configureLogging(logging: LogLevel | string | ILogger): HubConnectionBuilder { | ||
mikaelm12 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Arg.isRequired(logging, "logging"); | ||
|
||
if (isLogger(logging)) { | ||
this.logger = logging; | ||
} else if (typeof logging === "string") { | ||
const logLevel = parseLogLevel(logging); | ||
this.logger = new ConsoleLogger(logLevel); | ||
} else { | ||
this.logger = new ConsoleLogger(logging); | ||
} | ||
|
@@ -92,7 +128,7 @@ export class HubConnectionBuilder { | |
// Flow-typing knows where it's at. Since HttpTransportType is a number and IHttpConnectionOptions is guaranteed | ||
// to be an object, we know (as does TypeScript) this comparison is all we need to figure out which overload was called. | ||
if (typeof transportTypeOrOptions === "object") { | ||
this.httpConnectionOptions = {...this.httpConnectionOptions, ...transportTypeOrOptions}; | ||
this.httpConnectionOptions = { ...this.httpConnectionOptions, ...transportTypeOrOptions }; | ||
} else { | ||
this.httpConnectionOptions = { | ||
...this.httpConnectionOptions, | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 exported now that it's included in the public configureLogging type signature?
It might be better to change configureLogging to take akeyof typeof LogLevel
instead ofkeyof typeof LogLevelNameMapping
. You can still use the LogLevelNameMapping and call toLowerCase on the argument.As an upside, hat would be make things more consistent with the capitalization of our HubConnectionStates.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyof typeof LogLevel
doesn't work since we want to allow "warn" and "info". I say don't export anything new, and change the configureLogging type signature to take a string.We could link to the "Configure logging" section of docs from the configureLogging doc comment if the want to see what string are valid. If you want type safety, you can still use the Loglevel overload.