feat: mixin extension #7

virkt25 · 2017-09-29T18:27:35Z

A starter mixin extension that implements a simple .logger() function to bind a Logger automatically to logger. prefix.

connect to loopbackio/loopback-next#525

kjdelisle

A couple very minor items, but otherwise LGTM.
I"ll mark it as approved so you don't need to wait for me to dismiss, but I do think there should be changes to the items mentioned.

kjdelisle · 2017-09-29T19:12:41Z

src/logger-mixin/mixins/README.md

+
+### LoggerMixin
+
+LoggerMixin adds capabilities to a LoopBack Next Application by adding a `.logger()` function that allows used to bind a Logger class to `Context` automatically. The binding key will be `loggers.${Class.name}` where `Class.name` is the name of the Logger class being bound. The Mixin also overrides existing `.component()` function so that components are also capable of providing Logger's to be bound automatically. 


...function that allows used to... --> that is used to or that allows a

...also capable of providing Logger's to be... --> also capable of providing their own Logger implementations automatically

Also, split this line into multiple lines, it's far too long.

kjdelisle · 2017-09-29T19:16:32Z

src/logger-mixin/mixins/README.md

+
+Once a Logger has been bound, you can retrieve it by using [Dependency Inject](http://loopback.io/doc/en/lb4/Dependency-injection.html)
+
+**More Examples for binding a Logger**


#### More Logger Examples

kjdelisle · 2017-09-29T19:20:23Z

src/logger-mixin/mixins/logger-mixin.ts

+* class MyApplication extends LoggerMixin(Application) {}
+* ```
+*/
+export function LoggerMixin<T extends Constructor<any>>(superClass: T) {


If this is meant to be used with Applications (or implementations of Application), then it should be of type Constructor<Application>.

+1 for using a more specific type than any.

I don't think we can use Application here, because we may be referring to a different constructor class (in our copy of @loopback/core in node_modules tree) than the constructor class used by the application using our mixin (from their copy of @loopback/core in their node_modules tree).

Considering that our own @loopback/repository is using T extends Constructor<any> (source), I think it's ok to do the same here, at least for now.

Using type Application throws a whole series of errors such as using internal type ApplicationOptions, etc. I think it's because we don't have an interface for Application. Even setting it to `Constructor<{}> results in errors.

kjdelisle · 2017-09-29T19:24:31Z

src/logger-mixin/mixins/logger-mixin.ts

+      if (this.options.components) {
+        // Super would have already mounted the component
+        for (const component of this.options.components) {
+          this.component(component);


this.component is already called in the base Application constructor, which means you should be able to do this:

if (this.options.components) { for (const component of this.options.components) { mountComponentLogger(component); } }

This is not needed at all. For the mixin to work correctly, it's enough to override component method (as is already done below).

Application's constructor, invoked via super(...args) at the top of this method, will call this.component, there is no need to call it explicitly again. Unless I am missing something about how the prototypal inheritance and class keyword works in JavaScript?

Using type Application throws a whole series of errors such as using internal type ApplicationOptions, etc. I think it's because we don't have an interface for Application. Even setting it to `Constructor<{}> results in errors.

EDIT: This was meant for comment RE: Constructor type being any

bajtos

Your pull request mixes changes in project infrastructure (how files are transpiled, etc.) with the code for mixin extension. Perhaps the infrastructure changes were accidental? Please remove the infrastructure changes from this pull request and send them in a standalone PR if they are intentional.

bajtos · 2017-10-02T09:31:58Z

index.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+export * from './src';


What is the purpose of this file, why is it needed?

I'm unable to complete a build without this file as it results in errors with exported members not being found. We have it in all loopback-next/packages/* directories as well.

Thank you for the explanation. It turns out my original project setup in #2 have more flaws than just a missing index.ts, I extracted your fixes into a standalone pull request #10 - PTAL.

bajtos · 2017-10-02T09:32:53Z

package.json

    "lint": "tslint -c tslint.full.json --project tsconfig.json --type-check",
    "lint:fix": "npm run lint -- --fix",
    "prepublish": "npm run build",
-    "pretest": "npm run build",
+    "pretest": "npm run clean && npm run build",


Is it a good idea to run clean task before each test run? What if npm run build was able to do an incremental rebuild?

The issue I ran into with the build was that if I had a file somefile.ts and had done a build, then if I deleted that file, added a new file somenewfile.ts and then ran build again (without clean) then the dist folder would have both files in it.

Reference Issue: microsoft/TypeScript#13722

bajtos · 2017-10-02T09:33:31Z

src/controllers/README.md

@@ -1,3 +0,0 @@
-# Controllers
-
-This directory contains source files for the controllers exported by this extension.


Can you preserve this file please? Without it, the entire controllers directory will be removed by git. Unless that's what you are trying to accomplish?

bajtos · 2017-10-02T10:18:01Z

package.json

@@ -43,6 +41,7 @@
    "@loopback/testlab": "^4.0.0-alpha.7",
    "@types/mocha": "^2.2.43",
    "mocha": "^3.5.3",
+    "ts-node": "^3.3.0",


As of #2, we are not using ts-node. We are transpiling both production and test code in one go. In the development workflow, developer should start tsc in watch mode in background - that way files in dist are always up to date with the typescript sources. The tests are run via regular mocha then.

The project setup works well for people using VS Code. IIRC, you mentioned @virkt25 that you are using a different editor/IDE. To support your preference, we should add a new npm script for running tsc in watch mode, e.g. "tsc-watch": "tsc --target es2017 --outDir dist --watch". Then the instructions become:

Start the build process in background via npm run tsc-watch. It will run TypeScript compiler in backround, watching and recompiling files as you change them.

Run npm run vscode-test to re-run the test suite and lint the code for both programming and style errors.

I created a pull request to describe dev setup for people not using VS Code - see #8.

bajtos · 2017-10-02T10:24:26Z

src/logger-mixin/index.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+export * from './mixins/logger-mixin';


In #2, I laid out a directory structure where we have directories for different entities (bindings, mixins, repositories, controllers) at the first level, and then individual source files are nested in these directories. This is a convention I'd like to promote for all kinds of LoopBack-Next projects - loopback-next monorepo packages, extension packages, applications.

In that layout, your mixin file should go to src/mixins/logger-mixin.ts.

Here is how I envision the usage: as a developer wishing to write a new extension that provides a mixin (plus few more things like sequence action bindings), I'll rename logger-mixin.ts to a different name matching my extension intent (e.g. grpc-mixin.ts), do the same for other templates/examples I'd like to leverage (e.g. rename the template/example of a binding from. foobar-provider.ts to invoke-method-provider.ts, etc.) and have a working component.

What I'd personally like to avoid is the necessity to move files and directories around, e.g. move src/logger-mixin/mixins/logger-mixin.ts' to src/mixins/grpc-mixin.ts, because it involves too many changes in other places (various index.ts` files, related test files, etc.).

bajtos · 2017-10-02T10:32:29Z

src/logger-mixin/mixins/logger-mixin.ts

+      if (this.options.components) {
+        // Super would have already mounted the component
+        for (const component of this.options.components) {
+          this.component(component);


This is not needed at all. For the mixin to work correctly, it's enough to override component method (as is already done below).

Application's constructor, invoked via super(...args) at the top of this method, will call this.component, there is no need to call it explicitly again. Unless I am missing something about how the prototypal inheritance and class keyword works in JavaScript?

bajtos · 2017-10-02T10:33:40Z

test/mocha.opts

 --recursive
 --reporter dot
-dist/test


Please revert changes in this file, they are breaking the setup introduced by #2.

bajtos · 2017-10-02T10:35:03Z

test/unit/logger-mixin.test.ts

+    });
+
+    expectComponentToBeBound(myApp, EmptyTestComponent);
+  });


I find this test extraneous - the fact that Application's constructor invokes app.component() is already tested in @loopback/core. No need to test it again here.

bajtos · 2017-10-02T10:35:55Z

test/unit/logger-mixin.test.ts

+    });
+
+    const logger = myApp.getSync('loggers.MyLogger');
+    expect(typeof logger.log).to.be.eql('function');


I think it's better to verify that the logger is an instance of MyLogger?

bajtos · 2017-10-02T10:36:36Z

tsconfig.json

@@ -9,13 +9,10 @@
    "lib": ["es2017", "dom"],
    "module": "commonjs",
    "moduleResolution": "node",
-    "target": "es2017",
+    "target": "es6",


Please revert.

While I have reverted the reason for this change was that package.json says it supports node >= 6 ... using es2017 means we can't support Node v6 as async & await doesn't work with target set to es2017. I'd say we move to support only node >=8 (which will be the official support target for LBNext).

virkt25 · 2017-10-03T03:33:29Z

@bajtos feedback applied. I'll remove the files modified in #8 (just have them here for CI to work).

Will remove `package.json` once #8 has landed.

bajtos

The changes look much better now.

bajtos · 2017-10-03T12:43:04Z

.vscode/tasks.json

-      "command": "tsc",
-      "args": ["--watch"],
+      "command": "npm",
+      "args": ["--silent", "run", "build:watch"],


I find this change suspicious, I hope it will go away after you rebase this patch on the latest master (that will include #10).

it should. will be rebasing shortly.

bajtos · 2017-10-03T12:50:01Z

src/mixins/README.md

+
+## Overview
+
+A mixin is a class which take the input of a base class and adds / modifies existing functions / status values and returns a new class which can be instantiated. The type of the returned class will still be the base class. Mixins let you extend the BaseClass by adding more functions to it, overriding existing ones or modifiying their behavior.


I am concerned that the content in this README is missing a clear explanation why extension developers would want/need to write an application mixin, and what are the necessary steps/features that such mixin should do in order to be consistent with other similar mixins. I would like to see a short paragraph explaining the different ways how a configuration can be provided to a LoopBack app (application config, component config, app.logger() helper) and perhaps explaining how to implement each option via the mixin.

On the second thought, I think such content should be in http://loopback.io/doc/en/lb4/Creating-components.html and this README should link to it.

The type of the returned class will still be the base class

This is not true, the returned class will be a child class inheriting from the original base class.

bajtos · 2017-10-03T12:50:08Z

src/mixins/README.md

+
+**Example**
+```
+cosnt newClass = MixinClass(BaseClass);


typo, should be const

I think we should be recommending class-based approach in our examples.

class MyApp extends MixinClass(Application) { }

One of the downsides of your current example is that we have a class constructor stored in a variable with camelCase name, when the convention for class constructors is to use PascalCase names.

(Same comments apply to the example below.)

bajtos · 2017-10-03T12:53:06Z

src/mixins/README.md

+  error(...args: any[]) {
+    const data = args.join(' ');
+    console.log('\x1b[31m error: ' + data + '\x1b[0m');
+  }


In order to keep these examples shorter, I am proposing to reduce the number of logger methods to two: log and error. (This will require changes in the source code too.)

Code change will only apply to test cases as the mixin doesn't supply a Logger implementation.

bajtos · 2017-10-03T12:54:05Z

src/mixins/logger.mixin.ts

+    *
+    * @param component The component to mount Logger's of
+    */
+    mountComponentLogger(component: Constructor<any>) {


Since this is mounting possibly multiple logger, I think mountComponentLoggers is a more appropriate name.

bajtos · 2017-10-03T12:54:23Z

src/repositories/README.md

@@ -1,3 +0,0 @@
-# Repositories
-
-This directory contains code for repositories provided by this extension.


Can we preserve this file please?

bajtos · 2017-10-03T12:55:42Z

test/unit/logger-mixin.test.ts

+import {Application, Component} from '@loopback/core';
+import {Constructor} from '@loopback/context';
+
+describe('LoggerMixin', () => {


Please move this file to test/unit/mixins/logger.mixin.test.ts. Related discussions: #6 (comment) and #6 (comment)

bajtos · 2017-10-03T12:57:52Z

test/unit/logger-mixin.test.ts

+
+  class MyLogger {
+    // tslint:disable-next-line:no-any
+    log(...args: any[]) {


In order to avoid those tslint exceptions, I am proposing to define a type alias:

// tslint:disable-next-line:no-any type LogArgs = any[]; // usage log(...args: LogArgs) { // ... }

bajtos · 2017-10-03T12:59:44Z

test/unit/logger-mixin.test.ts

+
+  class AppWithLogger extends LoggerMixin(Application) {}
+
+  class MyLogger {


In a real world extension, I would expect an interface defining the expected logger API - this interface should be provided as part of the extension. A test implementation of a logger should be marked as implementing this interface. As a result, when the API contract of a logger changes, tsc will report errors for all test logger that don't match the new API yet.

bajtos · 2017-10-03T13:01:01Z

tslint.full.json

-    "extends": [
-      "./tslint.json"
-    ],
-    // This configuration files enabled rules which require type checking


The changes in this file look like pure whitespace and formatting updates. Can you revert them please, and perhaps send in a new PR?

Will remove `package.json` once #8 has landed.

bajtos · 2017-10-04T12:27:59Z

src/mixins/README.md

+```ts
+constructor(...args: any[]) {
+  super(args);
+}```


Typo: triple back-tick must be on a standalone line.

bajtos · 2017-10-04T12:30:12Z

src/mixins/README.md

+### High level example
+```ts
+const MixedClass = MyMixinClass(Application);
+const MultipleMixedClassesClass = MyMixinClass(MyMixinClass2(Application));


Could you please change one of these examples to show how a mixin can be used with class inheritance? E.g.

class MixedApp extends MyMixinClass(Application) { }

As I commented earlier, I think this is the usage we should be promoting, and it may not be obvious to all TypeScript developers that mixins can be invoked inside extends clause.

bajtos · 2017-10-04T12:31:07Z

src/mixins/README.md

+
+#### Example user experience
+```ts
+new LoggerMixin(Application)({


Sorry, this is not how most people are going to use a mixin. Please save the new app to a variable (or create a new class extending the mixed-in app) and then call new on this new variable/class.

bajtos · 2017-10-04T12:32:05Z

src/mixins/README.md

+  loggers: [ColorLogger];
+}
+
+const LoggingApplication = LoggerMixin(Application);


LoggingApplication has been already defined on line 109, I think this definition is redundant and should be removed.

bajtos · 2017-10-04T12:33:15Z

src/mixins/README.md

+      const app = this;
+  }
+
+  app.logger(ColorLogger);


This line should be moved outside of class LoggingApplication. The app variable is also not defined.

Did you perhaps want to call this.logger(ColorLogger) from the constructor?

bajtos · 2017-10-04T12:35:19Z

src/types.ts

+  error(...args: LogArgs): void;
+
+  info?(...args: LogArgs): void;
+  warn?(...args: LogArgs): void;


I find this logger interface with optional methods difficult to use. What is my code supposed to do when the logger instance I receive does not provide info method? Call log? And what if warn is missing, how should I log warnings?

If you feel that a logger class without info & warn is not complete, then please replace these two optional methods with a comment explaining that a typical logger would have more methods, but we are omitting them in order to keep this example/template smaller.

Will remove `package.json` once #8 has landed.

Initial implementation for a logger mixin that allows applications to bind a Logger automatically.

virkt25 · 2017-10-12T05:35:19Z

@bajtos feedback was applied. PTAL.

bajtos · 2017-10-12T09:32:00Z

src/mixins/README.md

+  error(...args: LogArgs) {
+    const data = args.join(' ');
+    // log in red color
+    console.log('\x1b[31m error: ' + data + '\x1b[0m');


Maybe console.log('\x1b[31m error:', ...args, '\x1b[0m') for consistency with the log method? args.join(' ') will convert object values to string via .toString(), console.log uses util.format instead.

bajtos · 2017-10-12T09:32:55Z

src/mixins/README.md

+
+
+## Examples for using LoggerMixin
+```


Please add ts suffix to enable syntax highlighting.

bajtos · 2017-10-12T09:33:55Z

src/mixins/README.md

+class LoggingApplication extends LoggerMixin(Application) {
+  constructor(...args: any[]) {
+    super(...args);
+    this.logger(ColorLogger);


I think this is not necessary, because LoggingComponent is already providing ColorLogger. Can we remove this line please? Am I missing something?

I guess I wasn't clear but I was trying to demonstrate the various ways an end user would use LoggerMixin. I've updated the examples to be distinct so hopefully that clears that up.

LMK your thoughts on including this section.

bajtos · 2017-10-12T09:36:37Z

test/acceptance/mixins/logger.acceptance.ts

+
+describe('logger.mixin (acceptance)', () => {
+  // tslint:disable-next-line:no-any
+  let app: any;


When app is any, IDEs are not able to offer any meaningful autocompletion and the compiler cannot verify correct usage. I am proposing to move the definition of LoggerApplication out of createApp so that we can use it here.

A simpler alternative (wich may not work, IDK), is to use LoggerMixin(Application) as the type of this variable.

let app: LoggerMixin(Application);

This unfortunately doesn't work. I even tried using typeof on this as well as <>. I think any works well here because multiple mixins can be combined and it becomes difficult to nail down the final shape of the Mixin.

An interesting thing to note however is that we can use the LoggerApplication as the type if the class is moved outside of the createApplication function which is what I've done & think this is the approach most users will take as well.

I did however come across this while doing some research: microsoft/TypeScript#12114

I haven't tried any of the generic mappings and from reading I'm not sure it's possible to create a generic enough type for multiple mixins applied to a class.

An interesting thing to note however is that we can use the LoggerApplication as the type if the class is moved outside of the createApplication function which is what I've done & think this is the approach most users will take as well.

👍

bajtos · 2017-10-12T09:40:23Z

test/acceptance/mixins/logger.acceptance.ts

+  let app: any;
+  let server: RestServer;
+  // tslint:disable-next-line:no-any
+  let spy: any;


I am pretty sure sinon provides a type that can be used for spies, see e.g. how we are using sinon.SinonStub in Thinking in LoopBack.

let spy: sinon.SinonSpy;

bajtos · 2017-10-12T09:40:55Z

test/acceptance/mixins/logger.acceptance.ts

+  beforeEach(createLogger);
+  beforeEach(createController);
+  beforeEach(getServerFromApp);
+  beforeEach(() => {


We try to use named hooks, see http://loopback.io/doc/en/contrib/style-guide.html#hooks

bajtos · 2017-10-12T09:41:09Z

test/acceptance/mixins/logger.acceptance.ts

+    spy = sinon.spy(console, 'log');
+  });
+
+  afterEach(() => {


bajtos

Lovely,

bajtos · 2017-10-13T13:24:51Z

src/mixins/README.md

+class LoggingApplication extends LoggerMixin(Application) {
+  constructor(...args: any[]) {
+    super(...args);
+    this.logger(ColorLogger);


virkt25 requested review from raymondfeng, bajtos and kjdelisle September 29, 2017 18:29

virkt25 self-assigned this Sep 29, 2017

virkt25 changed the title ~~Feat mixin~~ feat: mixin extension Sep 29, 2017

kjdelisle approved these changes Sep 29, 2017

View reviewed changes

bajtos suggested changes Oct 2, 2017

View reviewed changes

bajtos mentioned this pull request Oct 2, 2017

chore: dev setup for custom editors/IDEs #8

Merged

virkt25 added a commit that referenced this pull request Oct 3, 2017

Revert project structure changes as discussed in #7

73ed464

Will remove `package.json` once #8 has landed.

bajtos suggested changes Oct 3, 2017

View reviewed changes

virkt25 force-pushed the feat-mixin branch from 1149eaa to 966f76d Compare October 3, 2017 13:51

virkt25 added a commit that referenced this pull request Oct 4, 2017

Revert project structure changes as discussed in #7

5595fb1

Will remove `package.json` once #8 has landed.

bajtos suggested changes Oct 4, 2017

View reviewed changes

virkt25 added a commit that referenced this pull request Oct 5, 2017

Revert project structure changes as discussed in #7

1c3c7e1

Will remove `package.json` once #8 has landed.

virkt25 force-pushed the feat-mixin branch from 68de2d2 to 8ffc310 Compare October 5, 2017 00:27

virkt25 added a commit that referenced this pull request Oct 11, 2017

Revert project structure changes as discussed in #7

7f0873c

Will remove `package.json` once #8 has landed.

virkt25 force-pushed the feat-mixin branch 2 times, most recently from 1499538 to b1f867e Compare October 11, 2017 19:24

feat: logger mixin extension

0264fbf

Initial implementation for a logger mixin that allows applications to bind a Logger automatically.

virkt25 force-pushed the feat-mixin branch from b028aba to 0264fbf Compare October 11, 2017 19:37

bajtos reviewed Oct 12, 2017

View reviewed changes

virkt25 added 2 commits October 13, 2017 01:11

fixup! Apply feedback

04071bb

fixup! missed feedback

f8a90ad

bajtos reviewed Oct 13, 2017

View reviewed changes

bajtos approved these changes Oct 13, 2017

View reviewed changes

virkt25 merged commit b8df9ee into master Oct 13, 2017

virkt25 deleted the feat-mixin branch October 13, 2017 14:19


		### LoggerMixin

		LoggerMixin adds capabilities to a LoopBack Next Application by adding a `.logger()` function that allows used to bind a Logger class to `Context` automatically. The binding key will be `loggers.${Class.name}` where `Class.name` is the name of the Logger class being bound. The Mixin also overrides existing `.component()` function so that components are also capable of providing Logger's to be bound automatically.


		Once a Logger has been bound, you can retrieve it by using [Dependency Inject](http://loopback.io/doc/en/lb4/Dependency-injection.html)

		More Examples for binding a Logger

		@@ -1,3 +0,0 @@
		# Controllers

		This directory contains source files for the controllers exported by this extension.


		## Overview

		A mixin is a class which take the input of a base class and adds / modifies existing functions / status values and returns a new class which can be instantiated. The type of the returned class will still be the base class. Mixins let you extend the BaseClass by adding more functions to it, overriding existing ones or modifiying their behavior.

		@@ -1,3 +0,0 @@
		# Repositories

		This directory contains code for repositories provided by this extension.


		class AppWithLogger extends LoggerMixin(Application) {}

		class MyLogger {

feat: mixin extension #7

feat: mixin extension #7

Conversation

virkt25 commented Sep 29, 2017 • edited Loading

kjdelisle 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

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

virkt25 Oct 2, 2017 • edited Loading

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

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 Oct 3, 2017

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

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

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

virkt25 commented Oct 12, 2017

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 commented Sep 29, 2017 •

edited

Loading

virkt25 Oct 3, 2017 •

edited

Loading

virkt25 Oct 2, 2017 •

edited

Loading