Fix #8974 by supporting strings in configureLogging for SignalR JS client #9252

analogrelay · 2019-04-10T16:35:07Z

This allows users to pass in a string for the log level when calling configureLogging. This makes it much easier to configure logging in environments where you don't have access to the enum definition (such as in Blazor when you're just doing this in a script block in your page).

Note: I also grouped all the configureLogging tests into their own describe because there were now quite a few :).

Test output for HubConnectionBuilder.test.ts:

 PASS  signalr/tests/HubConnectionBuilder.test.ts (6.62s)
  HubConnectionBuilder
    √ withUrl throws if url is null (3ms)
    √ withHubProtocol throws if protocol is null
    √ withUrl throws if url is undefined
    √ withHubProtocol throws if protocol is undefined (1ms)
    √ builds HubConnection with HttpConnection using provided URL (12ms)
    √ can configure transport type
    √ can configure hub protocol (2ms)
    √ reconnectPolicy undefined by default (1ms)
    √ withAutomaticReconnect throws if reconnectPolicy is already set
    √ withAutomaticReconnect uses default retryDelays when called with no arguments (1ms)
    √ withAutomaticReconnect uses custom retryDelays when provided
    √ withAutomaticReconnect uses a custom IReconnectPolicy when provided
    configureLogging
      √ throws if logger is null (1ms)
      √ throws if logger is undefined
      √ accepts LogLevel.None
      √ accepts LogLevel.Critical
      √ accepts LogLevel.Error
      √ accepts LogLevel.Warning (1ms)
      √ accepts LogLevel.Information
      √ accepts LogLevel.Debug
      √ accepts LogLevel.Trace (1ms)
      √ accepts "critical" as an alias for LogLevel.Critical
      √ accepts "debug" as an alias for LogLevel.Debug (1ms)
      √ accepts "error" as an alias for LogLevel.Error
      √ accepts "info" as an alias for LogLevel.Information
      √ accepts "information" as an alias for LogLevel.Information (1ms)
      √ accepts "none" as an alias for LogLevel.None
      √ accepts "trace" as an alias for LogLevel.Trace
      √ accepts "warn" as an alias for LogLevel.Warning
      √ accepts "warning" as an alias for LogLevel.Warning (1ms)
      √ allows logger to be replaced (1ms)
      √ configures logger for both HttpConnection and HubConnection (2ms)
      √ does not replace HttpConnectionOptions logger if provided (1ms)

cc @rynowak

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts

analogrelay · 2019-04-11T03:58:32Z

🆙 📅

src/SignalR/clients/ts/signalr/src/Utils.ts

analogrelay · 2019-04-11T16:28:42Z

Update to fix build.

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

halter73 · 2019-04-11T18:10:11Z

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 = {


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.~~

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.

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

Co-Authored-By: anurse <[email protected]>

analogrelay · 2019-04-12T03:29:05Z

Feedback applied!

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

halter73 · 2019-04-12T23:23:43Z

src/SignalR/clients/ts/signalr/src/Utils.ts

@@ -146,27 +146,35 @@ export class SubjectSubscription<T> implements ISubscription<T> {
 /** @private */
 export class ConsoleLogger implements ILogger {
    private readonly minimumLogLevel: LogLevel;
+	// Public for testing purposes.


This comment was indented with a tab.

YARN : error : F:/workspace/_work/1/s/src/SignalR/clients/ts/signalr/src/Utils.ts[149, 1]: space indentation expected [F:\workspace\_work\1\s\src\SignalR\clients\ts\signalr\signalr.npmproj]

double nit: Add newline above.

halter73 · 2019-04-12T23:24:06Z

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

@@ -44,15 +69,17 @@ 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 {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.


YARN : error : F:/workspace/_work/1/s/src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts[72, 1]: Exceeds maximum line length of 300 [F:\workspace\_work\1\s\src\SignalR\clients\ts\signalr\signalr.npmproj]

halter73

LGTM. Just some tslint cleanup left.

analogrelay · 2019-04-13T21:18:36Z

:'(. I'll look at that on Monday

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

mikaelm12 · 2019-04-15T16:39:08Z

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

+    if (typeof mapping !== "undefined") {
+        return mapping;
+    } else {
+        throw new Error(`Unknown log level: ${name}`);


Nit: Do we want to preserve the casing on the user input here?

This does preserve that casing. I think that's the appropriate behavior here, since it's echoing what the user put in.

mikaelm12

Test Failure. It looks like configureLogging is trying to define one of the string log level args as an ILogger which causes the test to fail.

  FAIL signalr/tests/HubConnectionBuilder.test.ts
    ΓùÅ Test suite failed to run
  
      signalr\tests\HubConnectionBuilder.test.ts (203,39): Argument of type '"trace" | "debug" | "info" | "information" | "warn" | "warning" | "error" | "critical" | "none"' is not assignable to parameter of type 'ILogger'.
        Type '"trace"' is not assignable to type 'ILogger'.
  
        at Object.runTsDiagnostics (common/node_modules/ts-jest/src/utils.ts:227:11)
        at process (common/node_modules/ts-jest/src/preprocessor.ts:47:5)
        at Object.process (common/node_modules/ts-jest/index.js:8:51)
        at ScriptTransformer.transformSource (common/node_modules/@jest/transform/build/ScriptTransformer.js:387:35)
        at ScriptTransformer._transformAndBuildScript (common/node_modules/@jest/transform/build/ScriptTransformer.js:457:40)
        at ScriptTransformer.transform (common/node_modules/@jest/transform/build/ScriptTransformer.js:513:25)

mikaelm12 · 2019-04-15T23:17:21Z

src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts

+            const mappedName = LogLevel[mapped];
+            it(`accepts "${str}" as an alias for LogLevel.${mappedName}`, async () => {
+                const builder = new HubConnectionBuilder()
+                    .configureLogging(str);


It's trying to assign str to an ILogger instead of a string here

analogrelay · 2019-04-17T15:09:24Z

Blocked on @mikaelm12 's change request ;).

Test was fixed

BrennanConroy · 2019-04-17T15:12:48Z

Unblocked now :D

analogrelay · 2019-04-17T15:15:47Z

mikaelm12 · 2019-04-17T16:07:44Z

This is why I don't like using the request changes feature lol

analogrelay · 2019-04-17T16:11:46Z

I only use it when I specifically feel the need to see the result of my feedback before signing off.

analogrelay added 2 commits April 10, 2019 08:54

Fix #8974 by adding support for string log levels.

238585d

update sample to use new string-based log level

9ef4b68

analogrelay added this to the 3.0.0-preview5 milestone Apr 10, 2019

analogrelay requested review from halter73, mikaelm12 and BrennanConroy April 10, 2019 16:35

BrennanConroy added the area-signalr Includes: SignalR clients and servers label Apr 10, 2019

halter73 reviewed Apr 10, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Show resolved Hide resolved

halter73 reviewed Apr 10, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Outdated Show resolved Hide resolved

halter73 reviewed Apr 10, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/tests/HubConnectionBuilder.test.ts Outdated Show resolved Hide resolved

feedback

557cbee

BrennanConroy approved these changes Apr 11, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/Utils.ts Show resolved Hide resolved

I CAN STILL CODE TRUST ME

406c054

analogrelay mentioned this pull request Apr 11, 2019

add docs in reaction to aspnet/AspNetCore#8974 dotnet/AspNetCore.Docs#11945

Merged

halter73 reviewed Apr 11, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Outdated Show resolved Hide resolved

halter73 reviewed Apr 11, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Outdated Show resolved Hide resolved

halter73 reviewed Apr 11, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

6ac8a5c

Co-Authored-By: anurse <[email protected]>

halter73 reviewed Apr 12, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Outdated Show resolved Hide resolved

Update src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts

eb930b6

halter73 reviewed Apr 12, 2019

View reviewed changes

halter73 approved these changes Apr 12, 2019

View reviewed changes

mikaelm12 reviewed Apr 15, 2019

View reviewed changes

src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts Show resolved Hide resolved

mikaelm12 reviewed Apr 15, 2019

View reviewed changes

fix build issues

53b5475

mikaelm12 previously requested changes Apr 15, 2019

View reviewed changes

fix again

09038c8

analogrelay merged commit 46db367 into master Apr 17, 2019

BrennanConroy deleted the anurse/8974-string-log-levels branch April 17, 2019 15:18

Fix #8974 by supporting strings in configureLogging for SignalR JS client #9252

Fix #8974 by supporting strings in configureLogging for SignalR JS client #9252

Uh oh!

Conversation

analogrelay commented Apr 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Apr 11, 2019

Uh oh!

Uh oh!

analogrelay commented Apr 11, 2019

Uh oh!

Uh oh!

halter73 Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

analogrelay commented Apr 12, 2019

Uh oh!

Uh oh!

halter73 Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 Apr 12, 2019

Choose a reason for hiding this comment

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Apr 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mikaelm12 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelm12 left a comment

Choose a reason for hiding this comment

Uh oh!

mikaelm12 Apr 15, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Apr 17, 2019

Uh oh!

BrennanConroy commented Apr 17, 2019

Uh oh!

analogrelay commented Apr 17, 2019

Uh oh!

mikaelm12 commented Apr 17, 2019

Uh oh!

analogrelay commented Apr 17, 2019

Uh oh!

Uh oh!

halter73 Apr 11, 2019 •

edited

Loading

halter73 Apr 11, 2019 •

edited

Loading

halter73 Apr 12, 2019 •

edited

Loading

analogrelay commented Apr 13, 2019 •

edited

Loading

analogrelay Apr 15, 2019 •

edited

Loading