From 46db3677602c5527346f7c067fba3b5c742120bb Mon Sep 17 00:00:00 2001 From: Andrew Stanton-Nurse Date: Wed, 17 Apr 2019 08:15:55 -0700 Subject: [PATCH] Fix #8974 by supporting strings in configureLogging for SignalR JS client (#9252) --- .../ts/signalr/src/HubConnectionBuilder.ts | 44 ++- src/SignalR/clients/ts/signalr/src/Utils.ts | 17 +- .../tests/HubConnectionBuilder.test.ts | 273 ++++++++++++------ .../samples/SignalRSamples/wwwroot/hubs.html | 2 +- 4 files changed, 240 insertions(+), 96 deletions(-) diff --git a/src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts b/src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts index 53940d9769..ef51ea7ebd 100644 --- a/src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts +++ b/src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts @@ -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, + 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}`); + } +} + /** 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 { 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, diff --git a/src/SignalR/clients/ts/signalr/src/Utils.ts b/src/SignalR/clients/ts/signalr/src/Utils.ts index 2c9174fade..1233ce6c51 100644 --- a/src/SignalR/clients/ts/signalr/src/Utils.ts +++ b/src/SignalR/clients/ts/signalr/src/Utils.ts @@ -147,8 +147,17 @@ export class SubjectSubscription implements ISubscription { 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 { @@ -156,17 +165,17 @@ export class ConsoleLogger implements ILogger { 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; } } diff --git a/src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts b/src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts index 6ee3fbb0af..76c25a8515 100644 --- a/src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts +++ b/src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts @@ -10,6 +10,7 @@ import { HubMessage, IHubProtocol } from "../src/IHubProtocol"; import { ILogger, LogLevel } from "../src/ILogger"; import { HttpTransportType, TransferFormat } from "../src/ITransport"; import { NullLogger } from "../src/Loggers"; +import { ConsoleLogger } from "../src/Utils"; import { VerifyLogger } from "./Common"; import { TestHttpClient } from "./TestHttpClient"; @@ -26,15 +27,51 @@ const commonHttpOptions: IHttpConnectionOptions = { logMessageContent: true, }; +// We use a different mapping table here to help catch any unintentional breaking changes. +// tslint:disable:object-literal-sort-keys +const ExpectedLogLevelMappings = { + 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, +}; + +class CapturingConsole { + public messages: any[] = []; + + public error(message: any) { + this.messages.push(CapturingConsole.stripPrefix(message)); + } + + public warn(message: any) { + this.messages.push(CapturingConsole.stripPrefix(message)); + } + + public info(message: any) { + this.messages.push(CapturingConsole.stripPrefix(message)); + } + + public log(message: any) { + this.messages.push(CapturingConsole.stripPrefix(message)); + } + + private static stripPrefix(input: any): any { + if (typeof input === "string") { + input = input.replace(/\[.*\]\s+/, ""); + } + return input; + } +} + registerUnhandledRejectionHandler(); describe("HubConnectionBuilder", () => { eachMissingValue((val, name) => { - it(`configureLogging throws if logger is ${name}`, () => { - const builder = new HubConnectionBuilder(); - expect(() => builder.configureLogging(val!)).toThrow("The 'logging' argument is required."); - }); - it(`withUrl throws if url is ${name}`, () => { const builder = new HubConnectionBuilder(); expect(() => builder.withUrl(val!)).toThrow("The 'url' argument is required."); @@ -111,98 +148,160 @@ describe("HubConnectionBuilder", () => { }); }); - it("allows logger to be replaced", async () => { - let loggedMessages = 0; - const logger = { - log() { - loggedMessages += 1; - }, - }; - const pollSent = new PromiseSource(); - const pollCompleted = new PromiseSource(); - const testClient = createTestClient(pollSent, pollCompleted.promise) - .on("POST", "http://example.com?id=abc123", (req) => { - // Respond from the poll with the handshake response - pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); - return new HttpResponse(202); - }); - const connection = createConnectionBuilder(logger) - .withUrl("http://example.com", { - ...commonHttpOptions, - httpClient: testClient, - }) - .build(); + describe("configureLogging", async () => { + function testLogLevels(logger: ILogger, minLevel: LogLevel) { + const capturingConsole = new CapturingConsole(); + (logger as ConsoleLogger).outputConsole = capturingConsole; - try { - await connection.start(); - } catch { - // Ignore failures + for (let level = LogLevel.Trace; level < LogLevel.None; level++) { + const message = `Message at LogLevel.${LogLevel[level]}`; + const expectedMessage = `${LogLevel[level]}: Message at LogLevel.${LogLevel[level]}`; + logger.log(level, message); + + if (level >= minLevel) { + expect(capturingConsole.messages).toContain(expectedMessage); + } else { + expect(capturingConsole.messages).not.toContain(expectedMessage); + } + } } - expect(loggedMessages).toBeGreaterThan(0); - }); - - it("uses logger for both HttpConnection and HubConnection", async () => { - const pollSent = new PromiseSource(); - const pollCompleted = new PromiseSource(); - const testClient = createTestClient(pollSent, pollCompleted.promise) - .on("POST", "http://example.com?id=abc123", (req) => { - // Respond from the poll with the handshake response - pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); - return new HttpResponse(202); + eachMissingValue((val, name) => { + it(`throws if logger is ${name}`, () => { + const builder = new HubConnectionBuilder(); + expect(() => builder.configureLogging(val!)).toThrow("The 'logging' argument is required."); }); - const logger = new CaptureLogger(); - const connection = createConnectionBuilder(logger) - .withUrl("http://example.com", { - ...commonHttpOptions, - httpClient: testClient, - }) - .build(); + }); - try { - await connection.start(); - } catch { - // Ignore failures + [ + LogLevel.None, + LogLevel.Critical, + LogLevel.Error, + LogLevel.Warning, + LogLevel.Information, + LogLevel.Debug, + LogLevel.Trace, + ].forEach((minLevel) => { + const levelName = LogLevel[minLevel]; + it(`accepts LogLevel.${levelName}`, async () => { + const builder = new HubConnectionBuilder() + .configureLogging(minLevel); + + expect(builder.logger).toBeDefined(); + expect(builder.logger).toBeInstanceOf(ConsoleLogger); + + testLogLevels(builder.logger!, minLevel); + }); + }); + + const levelNames = Object.keys(ExpectedLogLevelMappings) as Array; + for (const str of levelNames) { + const mapped = ExpectedLogLevelMappings[str]; + const mappedName = LogLevel[mapped]; + it(`accepts "${str}" as an alias for LogLevel.${mappedName}`, async () => { + const builder = new HubConnectionBuilder() + .configureLogging(str); + + expect(builder.logger).toBeDefined(); + expect(builder.logger).toBeInstanceOf(ConsoleLogger); + + testLogLevels(builder.logger!, mapped); + }); } - // A HubConnection message - expect(logger.messages).toContain("Starting HubConnection."); + it("allows logger to be replaced", async () => { + let loggedMessages = 0; + const logger = { + log() { + loggedMessages += 1; + }, + }; + const pollSent = new PromiseSource(); + const pollCompleted = new PromiseSource(); + const testClient = createTestClient(pollSent, pollCompleted.promise) + .on("POST", "http://example.com?id=abc123", (req) => { + // Respond from the poll with the handshake response + pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); + return new HttpResponse(202); + }); + const connection = createConnectionBuilder(logger) + .withUrl("http://example.com", { + ...commonHttpOptions, + httpClient: testClient, + }) + .build(); - // An HttpConnection message - expect(logger.messages).toContain("Starting connection with transfer format 'Text'."); - }); + try { + await connection.start(); + } catch { + // Ignore failures + } - it("does not replace HttpConnectionOptions logger if provided", async () => { - const pollSent = new PromiseSource(); - const pollCompleted = new PromiseSource(); - const testClient = createTestClient(pollSent, pollCompleted.promise) - .on("POST", "http://example.com?id=abc123", (req) => { - // Respond from the poll with the handshake response - pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); - return new HttpResponse(202); - }); - const hubConnectionLogger = new CaptureLogger(); - const httpConnectionLogger = new CaptureLogger(); - const connection = createConnectionBuilder(hubConnectionLogger) - .withUrl("http://example.com", { - httpClient: testClient, - logger: httpConnectionLogger, - }) - .build(); + expect(loggedMessages).toBeGreaterThan(0); + }); - try { - await connection.start(); - } catch { - // Ignore failures - } + it("configures logger for both HttpConnection and HubConnection", async () => { + const pollSent = new PromiseSource(); + const pollCompleted = new PromiseSource(); + const testClient = createTestClient(pollSent, pollCompleted.promise) + .on("POST", "http://example.com?id=abc123", (req) => { + // Respond from the poll with the handshake response + pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); + return new HttpResponse(202); + }); + const logger = new CaptureLogger(); + const connection = createConnectionBuilder(logger) + .withUrl("http://example.com", { + ...commonHttpOptions, + httpClient: testClient, + }) + .build(); - // A HubConnection message - expect(hubConnectionLogger.messages).toContain("Starting HubConnection."); - expect(httpConnectionLogger.messages).not.toContain("Starting HubConnection."); + try { + await connection.start(); + } catch { + // Ignore failures + } - // An HttpConnection message - expect(httpConnectionLogger.messages).toContain("Starting connection with transfer format 'Text'."); - expect(hubConnectionLogger.messages).not.toContain("Starting connection with transfer format 'Text'."); + // A HubConnection message + expect(logger.messages).toContain("Starting HubConnection."); + + // An HttpConnection message + expect(logger.messages).toContain("Starting connection with transfer format 'Text'."); + }); + + it("does not replace HttpConnectionOptions logger if provided", async () => { + const pollSent = new PromiseSource(); + const pollCompleted = new PromiseSource(); + const testClient = createTestClient(pollSent, pollCompleted.promise) + .on("POST", "http://example.com?id=abc123", (req) => { + // Respond from the poll with the handshake response + pollCompleted.resolve(new HttpResponse(204, "No Content", "{}")); + return new HttpResponse(202); + }); + const hubConnectionLogger = new CaptureLogger(); + const httpConnectionLogger = new CaptureLogger(); + const connection = createConnectionBuilder(hubConnectionLogger) + .withUrl("http://example.com", { + httpClient: testClient, + logger: httpConnectionLogger, + }) + .build(); + + try { + await connection.start(); + } catch { + // Ignore failures + } + + // A HubConnection message + expect(hubConnectionLogger.messages).toContain("Starting HubConnection."); + expect(httpConnectionLogger.messages).not.toContain("Starting HubConnection."); + + // An HttpConnection message + expect(httpConnectionLogger.messages).toContain("Starting connection with transfer format 'Text'."); + expect(hubConnectionLogger.messages).not.toContain("Starting connection with transfer format 'Text'."); + }); }); it("reconnectPolicy undefined by default", () => { @@ -228,7 +327,7 @@ describe("HubConnectionBuilder", () => { }); it("withAutomaticReconnect uses custom retryDelays when provided", () => { - const customRetryDelays = [ 3, 1, 4, 1, 5, 9 ]; + const customRetryDelays = [3, 1, 4, 1, 5, 9]; const builder = new HubConnectionBuilder() .withAutomaticReconnect(customRetryDelays); @@ -241,7 +340,7 @@ describe("HubConnectionBuilder", () => { }); it("withAutomaticReconnect uses a custom IReconnectPolicy when provided", () => { - const customRetryDelays = [ 127, 0, 0, 1 ]; + const customRetryDelays = [127, 0, 0, 1]; const builder = new HubConnectionBuilder() .withAutomaticReconnect(new DefaultReconnectPolicy(customRetryDelays)); diff --git a/src/SignalR/samples/SignalRSamples/wwwroot/hubs.html b/src/SignalR/samples/SignalRSamples/wwwroot/hubs.html index 859cac97bf..8700fd4d11 100644 --- a/src/SignalR/samples/SignalRSamples/wwwroot/hubs.html +++ b/src/SignalR/samples/SignalRSamples/wwwroot/hubs.html @@ -151,7 +151,7 @@ console.log('http://' + document.location.host + '/' + hubRoute); var connectionBuilder = new signalR.HubConnectionBuilder() - .configureLogging(signalR.LogLevel.Trace) + .configureLogging("trace") .withUrl(hubRoute, options) .withHubProtocol(protocol);