-
Notifications
You must be signed in to change notification settings - Fork 386
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
mixin and components #491
mixin and components #491
Conversation
@jannyHou I'll wait til this is a bit further along to review it OK? Pls ping me when it's ready for review. |
@crandmck yeah I am still adding stuff on local, thanks! |
_data/sidebars/lb4_sidebar.yml
Outdated
@@ -98,6 +98,15 @@ children: | |||
url: Crafting-LoopBack-4.html | |||
output: 'web, pdf' | |||
|
|||
- title: 'Language Related Concepts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth starting a new top-level section instead of nesting this new "Mixin" page under "Key concepts"? I don't know what's the right answer, just something to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos I feel at least now Mixin
is not a LB core concept, we don't have mixin level context, and don't reply on it to scaffold the app, it is more like a JS/TS strategy to extend class, which make other concepts extendable, like application, controller, as long as it's a class.
there is some context of this discussion, I pinged Raymond about moving mixin.ts
from @loopback/repository
to @loopback/core
, and he suggests we can have a @loopback/lang
or @loopback/common
module for JS/TS related stuff.
I created an issue for this idea: loopbackio/loopback-next#660
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S/be
Language-related concepts
Regarding new category |
pages/en/lb4/Lang-concepts.md
Outdated
@@ -0,0 +1,13 @@ | |||
--- | |||
lang: en | |||
title: 'Language Related Concepts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Language-related concepts
_data/sidebars/lb4_sidebar.yml
Outdated
@@ -98,6 +98,15 @@ children: | |||
url: Crafting-LoopBack-4.html | |||
output: 'web, pdf' | |||
|
|||
- title: 'Language Related Concepts' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S/be
Language-related concepts
pages/en/lb4/Creating-components.md
Outdated
@@ -228,6 +230,33 @@ export class AuthenticationProvider { | |||
} | |||
``` | |||
|
|||
## Extends Application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section has many grammatical and stylistic errors. Rather than go through them laboriously here, I'd prefer to either edit the text in thebranch directly or wait til the PR lands and then edit the text (none of the changes would require technical review) in master (ie. gh-pages). @jannyHou LMK which you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @crandmck I think now this PR is still waiting for technical review, I will let you know when it's done but before merging.
pages/en/lb4/Creating-components.md
Outdated
@@ -228,6 +230,33 @@ export class AuthenticationProvider { | |||
} | |||
``` | |||
|
|||
## Extends Application | |||
|
|||
When bind a component to an app, you may want to mount the component's propertis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When bind a component to an app
--> When binding a component to an app- typo: propertis --> properties
pages/en/lb4/Creating-components.md
Outdated
|
||
When bind a component to an app, you may want to mount the component's propertis | ||
and methods to application level context. | ||
A stragety to do that is using mixin, which extends a class with new properties and methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- typo: stragety --> strategy
- Not sure if i understand what it means.. Is it sth like:
a strategy is to use mixin to extends a class with new properties and methods ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhmlau yep.
pages/en/lb4/Creating-components.md
Outdated
An example scenario would be an app has multiple components with repositories bound to each of them, | ||
you can use a `RepositoryMixin` to mount all of the repositories to the app. | ||
|
||
If you are not familiar with concept `mixin`, check [Mixin](Mixin.htm) to know more, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested change.. simply say sth like:
For more details, see Mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have the current version since that document is not a must read for people who already knows JS mixin
. A simplified version implies Mixin.md
contains something additional/different to the JS concept, but it doesn't :)
pages/en/lb4/Mixin.md
Outdated
## Define Mixin | ||
|
||
By defining a mixin, you create a mixin function that takes in a base class, | ||
and returns a new class extends the base class with new properties and methods mixed to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps: "returns a new class extends the base class.." --> "returns a new class extending the base class..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you said focus on the technical aspects, but the typos I left comments about are code style errors or outright syntax errors.
pages/en/lb4/Creating-components.md
Outdated
const request = require('request-promise-native'); | ||
const weatherUrl = | ||
'http://samples.openweathermap.org/data/2.5/weather?appid=b1b15e88fa797225412429c1c50c122a1' | ||
|
||
export class CurrentTemperatureProvider { | ||
async value() { | ||
export class CurrentTemperatureProvider implement Provider<number>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here in the code snippet, should be "implements"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
horrible typo :(
pages/en/lb4/Creating-components.md
Outdated
const uuid = require('uuid/v4'); | ||
|
||
class CorrelationIdProvider { | ||
class CorrelationIdProvider implement Provider<any>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implements
pages/en/lb4/Creating-components.md
Outdated
export class CurrentTemperatureProvider { | ||
async value() { | ||
export class CurrentTemperatureProvider implement Provider<number>{ | ||
async value():number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a space: async value(): number
pages/en/lb4/Creating-components.md
Outdated
constructor(@inject('http.request') request) { | ||
this.request = request; | ||
} | ||
|
||
value() { | ||
value():any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing a space: value(): any
51f7019
to
907a715
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my comments below.
In addition to them, please include reference to the following materials explaining ES2015 and TypeScript mixins in more details:
pages/en/lb4/Creating-components.md
Outdated
export class MyValueProvider { | ||
value() { | ||
export class MyValueProvider implements Provider<string>{ | ||
value(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should not be strictly necessary, I believe TypeScript is able to infer the return type from Provider
definition and/or the actual value returned by the code of this method. Unless I am mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ actually, the code examples in this file are all using JavaScript, not TypeScript - see js
after three backticks. I think you should revert type-related changes in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos good catch...I thought the doc is out-of-date so update it according to our latest tsdoc in provider.ts
. But didn't think it in the JS compatible direction.
Let me verify it.
pages/en/lb4/Creating-components.md
Outdated
const request = require('request-promise-native'); | ||
const weatherUrl = | ||
'http://samples.openweathermap.org/data/2.5/weather?appid=b1b15e88fa797225412429c1c50c122a1' | ||
|
||
export class CurrentTemperatureProvider { | ||
async value() { | ||
export class CurrentTemperatureProvider implement Provider<number>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implements
?
pages/en/lb4/Creating-components.md
Outdated
const uuid = require('uuid/v4'); | ||
|
||
class CorrelationIdProvider { | ||
class CorrelationIdProvider implement Provider<any>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implements
?
@@ -35,4 +35,4 @@ In general, components can contribute the following items: | |||
In the future (before the GA release), components will be able to contribute additional items: | |||
|
|||
- Models | |||
- [Repositories](Repositories.html) | |||
- [Repositories](Repositories.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated whitespace change, could you please revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird...I could not see where is the whitespace(difference), I copy paste the original file but diff is still here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a character formatting problem (was this file made on Windows or something? :P)
pages/en/lb4/Mixin.md
Outdated
import {BaseClass} from '<path_exports_BaseClass>'; | ||
import {FooMixin, BarMixin} from '<path_exports_Mixins>'; | ||
|
||
let newClass = MixinBuilder.mix(baseClass).with(FooMixin, BarMixin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example has an important problem: it's assigning a class constructor to a variable newClass
. The usual convention is to start class names with a capital letter, e.g. NewClass
. However, because we are using let
+ assignment instead of class
, the name NewClass
will trigger a tslint error about violating naming conventions for variables.
IMO, we should not promote MixinBuilder and tell people to compose mixins directly via regular JS/TS function invocation and class inheritance:
class NewClass extends FooMixin(BarMixin(baseClass)) {
}
Another important difference to consider:
In my proposed approach, the javascript runtime see a correct class name (NewClass
) and NewClass.toString()
operator returns class NewClass...
:
> NewClass.name
"NewClass"
> NewClass.toString()
"class NewClass extends Mixin1(Mixin2(BaseClass)) {
}"
Compare it with the result of MixinBuilder.mix().with()
, which incorrectly sets the constructor name to BaseClass
and where newClass.toString()
returns a fully mixed class definition that makes it difficult to see what have been added by what mixin:
> newClass.name
"BaseClass"
> newClass.toString()
"class extends superClass {
constructor() {
super(...arguments);
this.mixinProp2 = 'mixinProp2';
}
static staticMixinMethod2() {
return 'mixin2.static';
}
mixinMethod2() {
return 'mixin2';
}
}"
I am using the following unit-test code to verify my assumptions and produce sample output: https://github.com/strongloop/loopback-next/blob/568c6e2e7242949e1e5ece040bc3a54d83ff1411/packages/repository/test/unit/mixin/mixin.ts#L53
cc @raymondfeng as the author of MixinBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not promote MixinBuilder and tell people to compose mixins directly via regular JS/TS function invocation and class inheritance:
I wonder if one of the objectives of our MixinBuilder is to compose mixins for this use case. If that's the case, IMO shouldn't the constructor name and class def be preserved by MixinBuilder? (unless there is a reason I'm not aware of)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raymondfeng Could you take a look of comment https://github.com/strongloop/loopback.io/pull/491/files#r145687471? Thanks!
My understanding of MixinBuilder
is it provides a prettier syntax to compose mixins to a class, but @bajtos has a good point in comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed the idea from http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI
Compare it with the result of MixinBuilder.mix().with(), which incorrectly sets the constructor name to BaseClass and where newClass.toString() returns a fully mixed class definition that makes it difficult to see what have been added by what mixin:
This could be solved by defining newClass
as
newClass = class extends MixinBuilder.mix(BaseClass).with(Mixin1, Mixin2) {};
Then print out:
console.log(newClass.name);
// newClass
console.log(newClass.toString());
// class extends _1.MixinBuilder.mix(BaseClass).with(Mixin1, Mixin2) {}
The problem of MixinBuilder
from my test is: function with()
doesn't set its returned type as a class extends the BaseClass
:
click to see example test
let AppWithRepoMixin = class extends MixinBuilder.mix(Application).with(RepositoryMixin){};
const myApp = new AppWithRepoMixin();
const boundComponentsBefore = myApp.find('components.*').map(b => b.key);
fails when compiling, because compiler doesn't know myApp
is an instance of Application
, therefore doesn't know it has a prototype function find()
returning an Array
. The code calls map
with any
instead of Array
and fails.
This looks like a stop for people wanting to use it...
I am going to defer writing doc for MixinBuilder
and create an issue for it. I still love the fluent api and would like to dig more ⭐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to defer writing doc for MixinBuilder and create an issue for it. I still love the fluent api and would like to dig more ⭐️
@jannyHou could you please paste a link to that new issue here for posterity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos sure, sorry forgot to link it here: loopbackio/loopback-next#673
pages/en/lb4/Mixin.md
Outdated
|
||
It is a commonly used JavaScript/TypeScript strategy to extend a class with new properties and methods. | ||
|
||
*question: does our mixinBuilder allow interface extends classes?* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I understand this question. Interfaces are compile-time things that are used by TypeScript compiler to verify type correctness. They are omitted from the transpiled javascript output and they are not available at runtime.
I think it's possible to declare that an interface extends a class, which means that the interface must have all prototype members (properties & methods) of the class being extended. However, no implementation details (no JS code) from the class is inherited this way, it's all about compile-time shape of objects (the type system) only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have removed this, it is a question I am curious to know when reading typescript's class docuemnt:
search "Interfaces Extending Classes" on page https://www.typescriptlang.org/docs/handbook/interfaces.html
MixinBuilder
's input is typed to Class
(we defined in common-types.ts)
constructor(public baseClass: Class<any>) {}
So I am wondering would Interface
work as well.
pages/en/lb4/Mixin.md
Outdated
import {Class} from "@loopback/repository"; | ||
|
||
export function timeStampMixin<T extends Class<any>> (baseClass: T) { | ||
return class newClass extends baseClass { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the usual convention for creating mixins is to use anonymous class:
return class extends baseClass {
// ...
};
I think V8 is smart enough to infer a better class name in such case, most likely use baseClass's name. E.g. timeStampMixin(UserController)
returns a class called UserController
and timeStampMixin(ProductController)
returns a class called ProductController
, which is much better than having both calls return classes with the same name newClass
.
pages/en/lb4/Mixin.md
Outdated
import {Class} from "@loopback/repository"; | ||
|
||
function loggerMixin<T extends Class<any>> (baseClass: T) { | ||
return class newClass extends baseClass { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, just return class extends baseClass
pages/en/lb4/Mixin.md
Outdated
} | ||
} | ||
|
||
let AdvancedController = MixinBuilder.mix(SimpleController).with(timeStampMixin, loggerMixin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I commented above, for cases where the list of mixins is known at compile time, we should not be using MixinBuilder
.
pages/en/lb4/Mixin.md
Outdated
function loggerMixin<T extends Class<any>> (baseClass: T) { | ||
return class newClass extends baseClass { | ||
// add a new method `logger()` | ||
logger(str: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to give methods names that start with a verb, i.e. log(str: string)
in this case.
pages/en/lb4/Creating-components.md
Outdated
|
||
From Janny: | ||
|
||
I am going to add a Lite version of `RepositoryMixin`'s code as the end of this section |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think you have some options. You could:
- Give an overview of
RepositoryMixin
and point to its implementation - Give a brief overview of
RepositoryMixin
with code snippets like you said, and point to the source code at the end
I think the first option might suffice, given that Mixins are described in detail in their own page. IMO, the main points this section should cover would be:
- How component developers can leverage mixins
- Guideline on how they should use mixins
- other considerations that they should take into account
pages/en/lb4/Mixin.md
Outdated
let newClass = MixinBuilder.mix(baseClass).with(FooMixin, BarMixin); | ||
``` | ||
|
||
## Define Mixin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should walk readers through this section first before Mixin Builder
section. IMO I see it as introducing the concept first, then showing them how we make their lives easier with our MixinBuilder
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think of the sequence when wrote it. Eventually decided to introduce MixinBuilder
first:
MixinBuilder.mix(baseClass).with(FooMixin, BarMixin)
is a very straight forward syntax to give a new mixin learner a big picture of what is the relation between class and mixin, then they go into section#define-mixin to dig more into implementation and usage. And to tell people already know mixin how MixinBuilder
provides a prettier syntax to apply mixins.
4790edb
to
8b43c0f
Compare
Apply feedback in 8b43c0f:
|
@bajtos @kjdelisle feedback applied, PTAL again, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits.
pages/en/lb4/Creating-components.md
Outdated
@@ -228,6 +228,66 @@ export class AuthenticationProvider { | |||
} | |||
``` | |||
|
|||
## Extends Application with Mixin | |||
|
|||
When binding a component to an app, you may want to extend the app with component's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kjdelisle I think it should be the component's
whole sentence is:
When binding a component to an app, you may want to extend the app with the *component's* properties and methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see with the component's
in the existing snippet. ;)
I'm alright with that change as well.
pages/en/lb4/Mixin.md
Outdated
``` | ||
|
||
Check article [real mixins with javascript classes](http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/) | ||
to know more about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to learn more
.
pages/en/lb4/Mixin.md
Outdated
|
||
Now let's add mixins to it: | ||
|
||
- A time stamp mixin that adds a property `createdAt` to record when a controller instance get created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to a record when a controller instance is created
pages/en/lb4/Mixin.md
Outdated
|
||
- A time stamp mixin that adds a property `createdAt` to record when a controller instance get created. | ||
|
||
- A logger mixin that adds a method `log` to log the `string` you provide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A logger mixin to provide logging tools
@@ -35,4 +35,4 @@ In general, components can contribute the following items: | |||
In the future (before the GA release), components will be able to contribute additional items: | |||
|
|||
- Models | |||
- [Repositories](Repositories.html) | |||
- [Repositories](Repositories.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a character formatting problem (was this file made on Windows or something? :P)
8b43c0f
to
8c8890d
Compare
@kjdelisle thank you :) 8c8890d Really couldn't figure out how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more tiny fixes
pages/en/lb4/Creating-components.md
Outdated
properties and methods. | ||
This can be achieved by using mixin. | ||
|
||
If you are not familiar with concept `mixin`, check [Mixin](Mixin.htm) to know more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are not familiar with the mixin concept, see [Mixin](Mixin.htm) to learn more.
pages/en/lb4/Creating-components.md
Outdated
|
||
When binding a component to an app, you may want to extend the app with the component's | ||
properties and methods. | ||
This can be achieved by using mixin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by using mixins
be52102
to
837ee53
Compare
@slnode test please |
@slnode test please |
1 similar comment
@slnode test please |
connect to loopbackio/loopback-next#580
Mixin.md
Creating-component.md