fix: clean up example-log-extension #952

raymondfeng · 2018-02-01T20:31:18Z

This PR is a follow-up to the following issue based on review comments:
#939

Demonstrate how to plug in a different log (console vs memory)
Switch to @loopback/metadata for decorator implementation
Remove log level provider as it's supposed to be contributed by the app

The exercise also exposes a few needs:

Support for optional dependency so that we can fall back a default
implementation (or a better way) (See Improve dependency injection/resolution #946)
Allow a component to bind artifacts beyond providers (See [WIP] feat(context): Allow components to expose a list of bindings #929)

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

virkt25

Looks great! Just one comment below.

virkt25 · 2018-02-01T21:09:06Z

packages/example-log-extension/src/component.ts


 export class LogComponent implements Component {
  providers?: ProviderMap = {
    [EXAMPLE_LOG_BINDINGS.TIMER]: TimerProvider,
    [EXAMPLE_LOG_BINDINGS.LOG_ACTION]: LogActionProvider,
-    [EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL]: LogLevelProvider,


The purpose of having a LogLevelProvider was to provide a default log level when the component is bound. Should this not be the case anymore? If so, it should be updated in the readme.

As I mentioned in the PR description, the log level should be set by the app. Having a provider for the default value is overkill. There are two perspectives:

The LogAction has a optional (to be added by Improve dependency injection/resolution #946) dependency of level. The default is WARN.

From the component's view, forcing it to contribute a provider for a single default value is not ideal. That's why I'm proposing to allow components to expose bindings of any flavor in [WIP] feat(context): Allow components to expose a list of bindings #929.

I'll update the README.

I like the idea of changing the log level into an optional dependency that has the default value configured here in the component, and getting rid of LogLevelProvider along the way 👍

virkt25 · 2018-02-02T05:53:18Z

logToConsole is causing a decrease in test coverage and adding tests to it will cause console log messages to be printed in tests :/ What happens if we stub a function for testing and then restore it? Does it still print out the logs like it does when a spy is used?

This isn't a blocker but just wondering ...

raymondfeng · 2018-02-02T06:48:33Z

We can comment the code instructing istanbul to ignore this block. There is no point to artifically boost the coverage number.

bajtos

IIUC, the test suite is no longer verifying the behaviour of the default console logger. It would be good to have some integration or acceptance test using Sinon to intercept console.* calls and verify the implementation of the console logger too. However, it looks like this was not part of the original code, therefore it's most likely out of scope of your pull request.

I have few more comments, see below.

bajtos · 2018-02-02T08:50:57Z

packages/example-log-extension/README.md

  constructor(
    @inject.getter(CoreBindings.CONTROLLER_CLASS)
    private readonly getController: Getter<Constructor<{}>>,
    @inject.getter(CoreBindings.CONTROLLER_METHOD_NAME)
    private readonly getMethod: Getter<string>,
    @inject(EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL)
    private readonly logLevel: number,
+    // Use a getter to defer to resolution of Logger
+    @inject.getter(EXAMPLE_LOG_BINDINGS.LOGGER) public getLogger: Getter<LogFn>,


Please use property-based injection for dependencies that have reasonable default. That way users instantiating this Provider class directly (typically unit tests) don't have to repeat specification of default argument values (i.e. logToConsole).

See our own docs: http://loopback.io/doc/en/lb4/Dependency-injection.html#property-injection (emphasis is mine)

Property injection is usually used for optional dependencies which are not required for the class to function or for dependencies that have a reasonable default.

@raymondfeng I still think we should be using property-based injection here, it's the best practice we are recommending ourselves in our docs! If you disagree then please explain your reasoning.

bajtos · 2018-02-02T08:52:22Z

packages/example-log-extension/README.md

+      let logger: LogFn;
+      try {
+        // Try to see if an external logger is bound
+        // This is a workaround before optional injection is supported


This may not be necessary if property-based injection (see my comment above) already supports optional injection.

If it does not, then please add a link to the issue or pull request tracking the task of adding support for optional injection.

See #946 for optional injection. I'll rebase to #946 once it's merged.

bajtos · 2018-02-02T08:59:35Z

packages/example-log-extension/src/types.ts

@@ -23,4 +23,6 @@ export type LevelMetadata = {level: number};

 export type HighResTime = [number, number]; // [seconds, nanoseconds]

+export type LogFn = (msg: string, level: number) => void;


I am confused about the difference between LogFn and LogAction. Please add tsdoc comments to clarify.

bajtos · 2018-02-02T09:01:52Z

packages/example-log-extension/src/component.ts


 export class LogComponent implements Component {
  providers?: ProviderMap = {
    [EXAMPLE_LOG_BINDINGS.TIMER]: TimerProvider,
    [EXAMPLE_LOG_BINDINGS.LOG_ACTION]: LogActionProvider,
-    [EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL]: LogLevelProvider,


I like the idea of changing the log level into an optional dependency that has the default value configured here in the component, and getting rid of LogLevelProvider along the way 👍

bajtos · 2018-02-02T09:06:49Z

logToConsole is causing a decrease in test coverage and adding tests to it will cause console log messages to be printed in tests :/ What happens if we stub a function for testing and then restore it? Does it still print out the logs like it does when a spy is used?
We can comment the code instructing istanbul to ignore this block. There is no point to artifically boost the coverage number.

I don't consider tests verifying the console logger as artificial boosting of our coverage numbers, see my comment above.

IIUC, the test suite is no longer verifying the behaviour of the default console logger. It would be good to have some integration or acceptance test using Sinon to intercept console.* calls and verify the implementation of the console logger too. However, it looks like this was not part of the original code, therefore it's most likely out of scope of your pull request.

In that light, I think adding tests for the console logger should be a part of this pull request.

bajtos · 2018-02-02T09:07:50Z

@raymondfeng Thank you for opening this pull request, I really appreciate that you are cleaning up the outdated log-extension example and bringing it up to date with our latest APIs & best practices ❤️

raymondfeng · 2018-02-03T05:12:43Z

@bajtos PTAL.

bajtos · 2018-02-06T09:17:10Z

packages/example-log-extension/README.md

  constructor(
    @inject.getter(CoreBindings.CONTROLLER_CLASS)
    private readonly getController: Getter<Constructor<{}>>,
    @inject.getter(CoreBindings.CONTROLLER_METHOD_NAME)
    private readonly getMethod: Getter<string>,
    @inject(EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL)
    private readonly logLevel: number,
+    // Use a getter to defer to resolution of Logger
+    @inject.getter(EXAMPLE_LOG_BINDINGS.LOGGER) public getLogger: Getter<LogFn>,


@raymondfeng I still think we should be using property-based injection here, it's the best practice we are recommending ourselves in our docs! If you disagree then please explain your reasoning.

bajtos · 2018-02-06T09:22:40Z

packages/example-log-extension/src/types.ts

@@ -7,6 +7,9 @@

 import {ParsedRequest, OperationArgs} from '@loopback/rest';

+/**
+ * A function to perform REST req/res logging action
+ */
 export interface LogFn {


This is out of sync with README. README uses LogAction, the source code here uses LogFn. Please pick one name and use it consistently everywhere.

raymondfeng · 2018-02-06T19:53:29Z

I still think we should be using property-based injection here, it's the best practice we are recommending ourselves in our docs! If you disagree then please explain your reasoning.

I agree. But it depends on #946. I'll fix it once #946 is landed.

raymondfeng · 2018-02-06T23:57:02Z

@bajtos The optional injection of log level and writer is now implemented on top of #946.

raymondfeng · 2018-02-08T17:00:06Z

@bajtos PTAL

bajtos

Almost there!

bajtos · 2018-02-09T11:20:53Z

packages/example-log-extension/test/unit/providers/log-action.provider.unit.ts

+  });
+});
+
+async function getLogger(inMemory: boolean) {


Boolean arguments are an anti-pattern, the code getLogger(false) and getLogger(true) is difficult to understand.

I am proposing to change inMemory: boolean to logWriter?: LogWriterFn.

Usage:

// log to console getLogger(); // log to memory getLogger(logToMemory);

bajtos · 2018-02-09T11:22:48Z

packages/example-log-extension/src/providers/log-action.provider.ts

+  private logWriter: LogWriterFn = logToConsole;
+
+  @inject(EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL, {optional: true})
+  private logLevel: number = LOG_LEVEL.WARN;


Both logWriter and logLevel must be public, so that consumers of this class can customize them explicitly (without depending on Dependency Injection).

bajtos · 2018-02-09T11:24:50Z

packages/example-log-extension/test/unit/providers/log-action.provider.unit.ts

-  function timer(startTime?: HighResTime): HighResTime {
-    if (!startTime) return [3, 3];
-    else return [0, 100000002];
+  const context: Context = new Context();


Unit tests should not be using Context, they should pass arguments directly.

const provider = new LogActionProvider(TestClass, 'test', timer); if (logWriter) provider.logWriter = logWriter; return provider.value();

This approach has an important advantage - when the dependencies of the log action provider change, the TypeScript compiler will remind us to update this test. When using context to resolve dependencies, we won't discover the problem until runtime.

bajtos · 2018-02-09T11:27:08Z

packages/example-log-extension/src/providers/log-action.provider.ts

  constructor(
    @inject.getter(CoreBindings.CONTROLLER_CLASS)
    private readonly getController: Getter<Constructor<{}>>,
    @inject.getter(CoreBindings.CONTROLLER_METHOD_NAME)
    private readonly getMethod: Getter<string>,
-    @inject(EXAMPLE_LOG_BINDINGS.APP_LOG_LEVEL)
-    private readonly logLevel: number,
    @inject(EXAMPLE_LOG_BINDINGS.TIMER) public timer: TimerFn,


While we are cleaning up the dependencies - I think timer should be an optional dependency injected via a property, defaulting to the hrtime-based implementation. Thoughts?

(It's ok to leave this change out of scope of this pull request.)

bajtos · 2018-02-09T11:28:27Z

@virkt25 would you like to take another look too?

raymondfeng · 2018-02-09T23:18:35Z

@bajtos I fixed the unit test and leave timer as-is. PTAL.

raymondfeng · 2018-02-12T16:37:01Z

@bajtos PTAL.

bajtos

LGTM 👍

I have few comments to consider. No further review from me is needed even if you decide to address them.

bajtos · 2018-02-13T11:50:11Z

packages/example-log-extension/README.md

-          console.log(chalk.red(`ERROR: ${log}`));
-          break;
-      }
+      this.logWriter(msg, level);


Nitpick (feel free to ignore): this.write or this.writeLog would read more naturally (method names should start with a verb).

bajtos · 2018-02-13T11:51:20Z

packages/example-log-extension/src/decorators/log.decorator.ts

+      EXAMPLE_LOG_BINDINGS.METADATA,
+      controllerClass.prototype,
+      methodName,
+    ) || {level: LOG_LEVEL.OFF}


Would it make sense to extend getMethodMetadata with an additional argument allowing callers to specify the default value? (It's out of scope of this pull request, just asking.)

It can be useful especially when falsey metadata values are allowed, e.g. a number 0.

bajtos · 2018-02-13T11:53:51Z

packages/example-log-extension/test/unit/providers/log-action.provider.unit.ts

+    timer,
+  );
+
+  provider.logLevel = LOG_LEVEL.WARN;


Isn't this line redundant, considering that logLevel is set to WARN by default?

shimks · 2018-02-13T16:11:57Z

@raymondfeng I've just landed #975, so this PR probably needs to be rebased again (sorry)

This PR is a follow-up to the following issue based on review comments: #939 - Demonstrate how to plug in a different log (console vs memory) - Switch to @loopback/metadata for decorator implementation - Remove log level provider as it's supposed to be contributed by the app The exercise also exposes a few needs: 1. Support for optional dependency so that we can fall back a default implementation (or a better way) 2. Allow a component to bind artifacts beyond providers

raymondfeng requested review from bajtos and kjdelisle as code owners February 1, 2018 20:31

raymondfeng added the needs-priority label Feb 1, 2018

raymondfeng requested a review from virkt25 February 1, 2018 20:31

virkt25 reviewed Feb 1, 2018

View reviewed changes

raymondfeng force-pushed the improve-log-extension branch from 7cbaea6 to b89dd96 Compare February 1, 2018 21:33

bajtos suggested changes Feb 2, 2018

View reviewed changes

raymondfeng force-pushed the improve-log-extension branch 2 times, most recently from e194b06 to 2ff495b Compare February 3, 2018 05:11

bajtos suggested changes Feb 6, 2018

View reviewed changes

raymondfeng force-pushed the improve-log-extension branch 2 times, most recently from a86b2a3 to 8a54686 Compare February 6, 2018 23:50

raymondfeng force-pushed the improve-log-extension branch from 8a54686 to d062a5d Compare February 8, 2018 05:05

bajtos suggested changes Feb 9, 2018

View reviewed changes

bajtos requested a review from virkt25 February 9, 2018 11:28

raymondfeng force-pushed the improve-log-extension branch 2 times, most recently from c36c6f8 to 420111c Compare February 9, 2018 23:18

virkt25 approved these changes Feb 12, 2018

View reviewed changes

bajtos mentioned this pull request Feb 13, 2018

Decouple application config from features (e.g. components) #975

Merged

6 tasks

bajtos approved these changes Feb 13, 2018

View reviewed changes

raymondfeng force-pushed the improve-log-extension branch from 420111c to 270c83a Compare February 13, 2018 16:27

raymondfeng merged commit f13f603 into master Feb 13, 2018

raymondfeng removed the needs-priority label Feb 13, 2018

raymondfeng deleted the improve-log-extension branch February 22, 2018 05:08

		@@ -23,4 +23,6 @@ export type LevelMetadata = {level: number};

		export type HighResTime = [number, number]; // [seconds, nanoseconds]

		export type LogFn = (msg: string, level: number) => void;

fix: clean up example-log-extension #952

fix: clean up example-log-extension #952

Conversation

raymondfeng commented Feb 1, 2018 • edited by bajtos Loading

Checklist

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 commented Feb 2, 2018

raymondfeng commented Feb 2, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 2, 2018

bajtos commented Feb 2, 2018

raymondfeng commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Feb 6, 2018

raymondfeng commented Feb 6, 2018

raymondfeng commented Feb 8, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 9, 2018

raymondfeng commented Feb 9, 2018

raymondfeng commented Feb 12, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks commented Feb 13, 2018

raymondfeng commented Feb 1, 2018 •

edited by bajtos

Loading