-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
docs: dynamic datasource, model, repository, and controller #5630
Conversation
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.
Some minor comments. I reviewed mostly from the readers' perspective. Please make sure we get at least one technical review/approval before we land. Thanks.
docs/site/Repositories.md
Outdated
|
||
#### 2. Using a Repository mixin | ||
|
||
Create a repository mixin with your customization as shown in the " |
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.
Suggestion: since it will be a link, i think we don't need the double quotes.
docs/site/Repositories.md
Outdated
``` | ||
|
||
Note, the `app.repository()` method will be available only on `RepositoryMixin` | ||
apps |
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.
Application 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.
Nah, it's not available on the Application
class, it has to be composed with RepositoryMixin
.
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.
Right. I was thinking whether there's a better description than "RepositoryMix apps". I think it can be read it as Application class with RepositoryMixin?
d5a3a8e
to
dd4ead4
Compare
and modify it according to your requirements. | ||
|
||
For details about `defineCrudRestController` and `CrudRestControllerOptions`, | ||
refer to the [@loopback/rest-crud API documentation](./apidocs/rest-crud.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.
Maybe we should mention Class factory to allow parameterized decorations per https://loopback.io/doc/en/lb4/core-tutorial-part10.html (Class factory to allow parameterized decorations).
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.
+1
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.
Not really convinced. If I am understanding correctly, the content at the link describes a general programming pattern, not something specific to Controllers. If that's the case, we should avoid adding non-specific content to avoid noise.
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.
The class factory is part of the programming model pattern and it's being asked by our users. It gives our users more control. The change is applying to Controllers.md
and I don't think that's noise.
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.
The change is applying to
Controllers.md
Got the context, I thought it was about runtime controllers.
Added Class factory to allow parameterized decorations at an appropriate location.
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.
Nice! I agree with the general direction, let's improve the details now.
It would be great to have an overview of different ways how a model/repository/controller can be created (via lb4 repository
, via lb4 rest-crud
, dynamically at runtime). I was envisioning that Models.md/Controllers.md/etc. could have a short section explaining these different flavor. However, I understand such content may be out of scope of your current work, so I am fine with leaving this part for later.
and modify it according to your requirements. | ||
|
||
For details about `defineCrudRestController` and `CrudRestControllerOptions`, | ||
refer to the [@loopback/rest-crud API documentation](./apidocs/rest-crud.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.
+1
docs/site/DataSources.md
Outdated
|
||
A datasource can be created at runtime by creating an instance of | ||
`juggler.DataSource`. It requires a name for the datasource, the connector, and | ||
a connection url as shown below. |
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 find this misleading. The configuration object accepts any valid fields, e.g. host
, port
and user
. The connection url is just one of the many valid ways how to configure a datasource.
Please clarify.
I think there is no need to repeat all details here, we should instead add a link to an existing documentation page showing different datasource configuration options.
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.
Updated to make the statement more generic and linked to https://apidocs.strongloop.com/loopback-datasource-juggler/#datasource.
There's no LB4 doc with details about DataSource
options. Ideally it should be on https://loopback.io/doc/en/lb4/DataSources.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.
There's no LB4 doc with details about
DataSource
options. Ideally it should be on https://loopback.io/doc/en/lb4/DataSources.html.
Perhaps you can open a new issue for that and add it to "Documentation improvements" epic #5113?
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.
Opened yesterday - #5646.
Models can be created at runtime using the `defineModelClass()` helper function | ||
from the `@loopback/repository` class. It expects a base model to extend | ||
(typically `Model` or `Entity`), folowed by the model definition object as shown | ||
in the example below. |
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.
Can you please explain what is the second type parameter for?
docs/site/DataSources.md
Outdated
|
||
A datasource can be created at runtime by creating an instance of | ||
`juggler.DataSource`. It requires a name for the datasource, the connector, and | ||
a connection url as shown below. |
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.
There's no LB4 doc with details about
DataSource
options. Ideally it should be on https://loopback.io/doc/en/lb4/DataSources.html.
Perhaps you can open a new issue for that and add it to "Documentation improvements" epic #5113?
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.
Almost there!
docs/site/Model.md
Outdated
in the example below. | ||
(typically `Model` or `Entity`), followed by a `ModelDefinition` object as shown | ||
in the example below. The `ModelDefinition` object is an abstraction for | ||
specifying the various attributes of a LoopBack model. |
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.
Sorry for not being clear enough.
Can you please explain what is the second type parameter for?
In my comment ☝️, I was referring to the second item in the list of generic parameters for defineModelClass
, i.e. the inline anonymous interface {id: number; title?: string}
.
Now that I am looking at the example below again, I think may be good to show how bookDef
is created too.
const bookDef = new ModelDefinition('Book')
.addProperty('id', {type: 'number', id: true})
.addProperty('title', {type: 'string'});
const BookModel = // ...
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 afraid this comment is still not addressed.
As a user not familiar with defineModelClass, I don't understand what properties it should describe and why. Where is id and title coming from, why they have the particular type (number, string) and why title is optional while id is required?
I am looking for an explanation that
- the interface is providing TypeScript description for the properties of the model we are defining
- when omitted, the class will have only members inherited from the base model class, which typically means no properties
b7094b4
to
3f416ec
Compare
@bajtos thanks for the feedback, applied them. |
453691d
to
1fc720d
Compare
572f9f4
to
a446f43
Compare
Describe how to create datasources, models, repositories, and controllers at runtime.
a446f43
to
051d5b6
Compare
Hi will there be a section about creating dynamic relation for repo with inclusion resolver ? Is this even possible ? |
Describe how to create datasources, models, repositories, and
controllers at runtime. Part of addressing #4296.
First half addressed by #5591.