Skip to content

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

Merged
merged 8 commits into from
Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 40 additions & 4 deletions src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Copy link
Member

@halter73 halter73 Apr 11, 2019

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 a keyof typeof LogLevel instead of keyof 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.

Copy link
Member

@halter73 halter73 Apr 11, 2019

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.

trace: LogLevel.Trace,
debug: LogLevel.Debug,
info: LogLevel.Information,
information: LogLevel.Information,
warn: LogLevel.Warning,
warning: LogLevel.Warning,
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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@analogrelay analogrelay Apr 15, 2019

Choose a reason for hiding this comment

The 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 */
Expand Down Expand Up @@ -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 {
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);
}
Expand Down Expand Up @@ -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,
Expand Down
17 changes: 13 additions & 4 deletions src/SignalR/clients/ts/signalr/src/Utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,26 +147,35 @@ export class SubjectSubscription<T> implements ISubscription<T> {
export class ConsoleLogger implements ILogger {
private readonly minimumLogLevel: LogLevel;

// Public for testing purposes.
public outputConsole: {
error(message: any): void,
warn(message: any): void,
info(message: any): void,
log(message: any): void,
};

constructor(minimumLogLevel: LogLevel) {
this.minimumLogLevel = minimumLogLevel;
this.outputConsole = console;
}

public log(logLevel: LogLevel, message: string): void {
if (logLevel >= this.minimumLogLevel) {
switch (logLevel) {
case LogLevel.Critical:
case LogLevel.Error:
console.error(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
this.outputConsole.error(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
break;
case LogLevel.Warning:
console.warn(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
this.outputConsole.warn(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
break;
case LogLevel.Information:
console.info(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
this.outputConsole.info(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
break;
default:
// console.debug only goes to attached debuggers in Node, so we use console.log for Trace and Debug
console.log(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
this.outputConsole.log(`[${new Date().toISOString()}] ${LogLevel[logLevel]}: ${message}`);
break;
}
}
Expand Down
Loading