-
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(repository): describe HasMany relation #1500
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.
Looks 🔥
I think each relation should have their own page, similar to how we have relation docs spread out in the lb2/3 docs.
Don't forget the sidebar entries as well
docs/site/Relations.md
Outdated
relation definition, certain constraints need to be honoured by the target model | ||
repository, and thus, we produce a constrained version of it as a navigational | ||
property on the source repository. The first relation type we have available is | ||
the `has many` relation which operates by constraining the target repository by |
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 probably just call has many
HasMany
like we do in lb3 documentation.
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 that we should probably extract out HasMany
into a 'relations list' where we show what relations lb4 supports. Something like this:
Supported relations:
- [HasMany](HasMany-relations.md or something along that line)
docs/site/Relations.md
Outdated
constructor and optionally a custom foreign key to store the relation metadata. | ||
The decorator logic also designates the relation type and tries to infer the | ||
foreign key on the target model (or `keyTo` in the relation metadata) as a | ||
sensible default value which is used in LoopBack 3. It also calls |
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.
What is the sensible default value 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.
actually why sensible
? :) is it better to omit this word?
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 if I wasn't clear. I think there should be a concrete formula for how the foreign key is inferred (camelcased target model name + Id)
docs/site/Relations.md
Outdated
`property.array()` to ensure that the type of the property is inferred properly | ||
as an array of the target model instances. The decorated property name is used | ||
as the relation name and stored as part of the source model definition's | ||
relation metadata. The following example shows how to define a `hasMany` |
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.
Line break here please
docs/site/Relations.md
Outdated
|
||
```js | ||
{ | ||
type: 2; // denotes hasMany relation |
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 clarify that this is an enum value
docs/site/Relations.md
Outdated
|
||
```ts | ||
{import} {Order} from '../models/order.model.ts'; | ||
{import} {Customer} from '../models/customer.model.ts'; |
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.
Nitpick: have the two imports above come from the same directory (../models
)
docs/site/Relations.md
Outdated
Customer, | ||
typeof Customer.prototype.id | ||
> { | ||
public orders: HasManyRepositoryFactory<Order, typeof Order.prototype.id>; |
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 should be `HasManyRepositoryFactory<Order, typeof Customer.prototype.id>
docs/site/Relations.md
Outdated
couple of steps involved to configure it and use it. On the source repository, | ||
the following are required: | ||
|
||
- Use [Dependency Injectition](Dependency-injection.md) to inject an instance 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.
Spelling error
docs/site/Relations.md
Outdated
content="/src/repositories/customer.repository.ts.ts" %} | ||
|
||
```ts | ||
{import} {Order} from '../models/order.model.ts'; |
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.
Remove the brackets around import
.
I think this is breaking the markdown rendering
docs/site/Relations.md
Outdated
|
||
Once the has many relation has been defined and configured, it can be accessed | ||
via controller methods to call the underlying constrained repository CRUD APIs | ||
and exposed in LoopBack's swagger UI once decorated with |
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 exposed as a route
is good enough here. Not really opposed to what you have here though
docs/site/Relations.md
Outdated
@post('/customers/{id}/order') | ||
async createCustomerOrders( | ||
@param.path.number('id') customerId: number, | ||
@requestBody() orderData: Partial<Order>, |
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 type needs to be Order here due to the limitation of requestBody
. This should be tracked until #1443 is fixed
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 good to see we're using Customer/Order example which aligns with the demo scenario we want to do for GA. :)
A few things to consider below. We can leave it out of this PR and have a separate task to track it.
- agree with @shimks to have separate pages for different relations. We can do that once we have more relation supported as well.
- it would be good to have a diagram to show the relationship between Customer/Order model and Customer/Order repository.
docs/site/HasMany-relation.md
Outdated
summary: | ||
--- | ||
|
||
## Defining a HasMany Relation |
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 logical separation of the sections makes perfect sense, but wondering if we can make the title/first sentence clearer to describe the purpose... to make it easier to read/understand.
IIUC,
- Defining a HasMany Relation - it's defining the hasMany relation at the model level using @hasmany decorator.
- Configuring a HasMany relation - configuring the relation at the repository level (something along the line). Does it control how the data is retrieved or how the payload look like?
- Using HasMany constrained repository in a controller - it's about the usage in controller.
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 proposing to start with a short overview providing a high-level list of steps needed to add a relation to a LB4 app. For example:
To add a HasMany relation to your LoopBack application, you need to perform the following steps:
1. Add a property to your model to access related model instances.
2. Modify the source model repository class to provide access to
a constrained target model repository
3. (...)
docs/site/sidebars/lb4_sidebar.yml
Outdated
- title: 'Decorators' | ||
url: Decorators.html | ||
output: 'web, pdf' | ||
children: |
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.
Does this work as intended? Shouldn't it be under Relations
?
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.
Good catch. Fixed :)
docs/site/HasMany-relation.md
Outdated
|
||
## Defining a HasMany Relation | ||
|
||
The first relation type we have available is the `hasMany` relation which |
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 probably reword this sentence so that we don't give precedence (ordering) to the relations we support
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.
What Diana has said
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.
@b-admike Great stuff! The document introduced how to define a relation + creates the crud repository + inject the repo in a controller in a well organized way.
Just a few comments. LGTM
docs/site/HasMany-relation.md
Outdated
|
||
{% include code-caption.html content="/src/models/customer.model.ts" %} | ||
|
||
```ts |
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.
Might be a quite subjective feeling, but I was willing to see the code example first when reading the document.
For example:
The definition of the
hasMany
relation is inferred by using thehasMany
decorator. The decorator takes in the target model class constructor and optionally a custom foreign key to store the relation metadata.
Without seeing the code, I find it a little bit hard to imagine how this decorator would look like and not that easy to follow its further details like what it takes in :-p
docs/site/HasMany-relation.md
Outdated
@post('/customers/{id}/order') | ||
async createCustomerOrders( | ||
@param.path.number('id') customerId: number, | ||
@requestBody() orderData: Order, // FIXME(b-admike): should be Partial<Order> once https://github.com/strongloop/loopback-next/issues/1443 is fixed |
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.
nitpick: maybe remove the comment code?
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 to remove or rework the comment.
BTW the linked issue has been closed in favour of #1179.
If we want to explain that orderData
should use a different type in the future, then please add a "note" banner after the code snippet and explain the issue in more details. (I think it is a good idea to explain this problem to readers.)
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 start!
docs/site/HasMany-relation.md
Outdated
summary: | ||
--- | ||
|
||
## Defining a HasMany Relation |
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 proposing to start with a short overview providing a high-level list of steps needed to add a relation to a LB4 app. For example:
To add a HasMany relation to your LoopBack application, you need to perform the following steps:
1. Add a property to your model to access related model instances.
2. Modify the source model repository class to provide access to
a constrained target model repository
3. (...)
permalink: /doc/en/lb4/HasMany-relation.html | ||
summary: | ||
--- | ||
|
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 start this documentation page with a short explanation what is HasMany
relation. I think we can pretty much copy the "Overview" section from https://loopback.io/doc/en/lb3/HasMany-relations.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.
✅
(this has been resolved by content added below the line 10)
docs/site/HasMany-relation.md
Outdated
@property({ | ||
type: 'string', | ||
}) | ||
name: 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.
name?: string;
Alternatively, mark the property as required:true
docs/site/HasMany-relation.md
Outdated
}) | ||
name: string; | ||
|
||
@hasMany(Order) orders: Order[]; |
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, I think orders
should be an optional property - for example, the default CRUD repository methods like find
or create
are not setting this property.
docs/site/HasMany-relation.md
Outdated
|
||
```js | ||
{ | ||
type: 2; // denotes hasMany relation (this is an enum value) |
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.
Hmm, this is not great - the code is more difficult to debug and even more difficult to write in JavaScript (in the future). We should switch to string-based enums, so that type
can be hasMany
.
If you prefer, then I am ok to leave this change out of DP3 as long as you create a new GH issue labelled as LB4 GA
and p1
.
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.
Done. The change was easy enough :-).
docs/site/Relations.md
Outdated
|
||
## Overview | ||
|
||
Model relation in LoopBack 3 is one of its powerful features which help users |
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 consider adding more content from https://loopback.io/doc/en/lb3/Creating-model-relations.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.
I thought the content there is still relevant so I've copied some stuff from the overview.
public orders: HasManyRepositoryFactory<Order, typeof Order.prototype.id>; | ||
public orders: HasManyRepositoryFactory< | ||
Order, | ||
typeof Customer.prototype.id |
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 you are using HasManyRepositoryFactory
with correct types (source model, target id). Let's fix the documentation too. (See one of my earlier comments above.)
docs/site/HasMany-relation.md
Outdated
``` | ||
|
||
The property type metadata is also preserved as an array of type `Order` as part | ||
of the decoration. Another usage of the decorator with a custom foreign key name |
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 property type metadata is also preserved as an array of type
Order
as part of the decoration.
I have no idea what does this mean for me as a LB4 user. Please explain this in the context of LB4 apps, controllers or repositories and perhaps add a code snippet to better illustrate what you mean.
docs/site/HasMany-relation.md
Outdated
} | ||
``` | ||
|
||
The metadata generated for the above decoration is as follows: |
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.
IMO, the metadata format is an implementation details that most (if not all) LoopBack users should not need to understand in order to build and troubleshoot their applications. I am proposing to remove this section from the docs and perhaps move it to API docs and/or convert into code comments in the appropriate place.
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 wrote this thinking that the docs might have a wider audience than our users or that users who are developers would like to know the info. But I agree, it doesn't really have a place here. I'll remove it for now and find an appropriate place for it as you have suggested.
docs/site/HasMany-relation.md
Outdated
|
||
{% include code-caption.html | ||
content="src/controllers/customer-orders.controller.ts" %} | ||
|
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 add a short paragraph explaining that:
- in LB3, methods for accessing related models were part of the same source model class
- in LB4, we recommend to create a new controller for each relation in order to keep the controller classes smaller. I.e. don't add order-related methods to
CustomerController
, but create a newCustomerOrdersController
class for them.
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.
LGTM in general. There are a few comments, you can pick the ones that are appropriate. Thanks.
docs/site/HasMany-relation.md
Outdated
|
||
## Defining a HasMany Relation | ||
|
||
To add a `HasMany` relation to your LoopBack application, you need to perform |
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 seems like the steps fit better in the overview, because this section is already about the first step. Just a suggestion
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.
Good catch! That is where it should be :-).
docs/site/HasMany-relation.md
Outdated
|
||
Model relations in LoopBack 4 are defined by having a decorator on a designated | ||
relational property. This section describes how to define a `hasMany` relation | ||
at the model level using the `hasMany` decorator. The relation constrains 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:
the hasMany
decorator -> the @hasMany
decorator
docs/site/HasMany-relation.md
Outdated
|
||
## Configuring a HasMany relation | ||
|
||
The configuration and resolution of a `HasMany` relation takes place at 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.
nitpick: we have been using hasMany
instead of HasMany
in the previous paragraph. it would be good to keep it consistent.
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 agree; I'll stick with hasMany
.
docs/site/HasMany-relation.md
Outdated
## Configuring a HasMany relation | ||
|
||
The configuration and resolution of a `HasMany` relation takes place at the | ||
repository level. Once `HasMany` relation is defined on the source model, then |
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.
same as above. HasMany
-> hasMany
aab3cbe
to
a9e258d
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.
Looks mostly good, see few minor comments below.
When cleaning up the history, please split the changes into multiple commits, so that documentation changes are not mixed with changes in the runtime APIs. This is important to allow our changelog generator to correctly capture all changes in the changelog.
Ideally, I think there should be following commits:
- Change RelationType from numeric to string enums
- Switch order of generic arguments of HasManyRepositoryFactory
- Add docs for HasMany relation
docs/site/HasMany-relation.md
Outdated
`HasManyRepositoryFactory<targetModel, typeof sourceModel.prototype.id>` on | ||
the source repository class. | ||
- call the `_createHasManyRepositoryFactoryFor` function in the constructor of | ||
the source repository class with the relation name (decorated relation propert |
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: propert
-> property
docs/site/HasMany-relation.md
Outdated
) {} | ||
|
||
@post('/customers/{id}/order') | ||
async createCustomerOrders( |
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 createOrderOfCustomer
would be a better name, considering that the first method argument is customerId
?
If not, then at least change the name into singular because we are creating a single order: createCustomerOrder
.
Another name to consider: createOrder
or simply create
- both the source and target model names are already captures in the controller name.
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.
Thank you for those suggestions @bajtos. I like either createOrderOfCustomer
or createOrder
. Let's go with createOrder
.
embedsOne = 'embedsOne', | ||
embedsMany = 'embedsMany', | ||
referencesOne = 'referencesOne', | ||
referencesMany = 'referencesMany', |
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.
Lovely 👍
Sounds good 💯 |
docs/site/HasMany-relation.md
Outdated
@@ -189,4 +189,18 @@ export class CustomerOrdersController { | |||
} | |||
``` | |||
|
|||
<!--- TODO: Add details about `orderData` variable typing issue ---> | |||
In LoopBack 3, methods for accessing related models were part of the source | |||
model class i.e. `customer.orders.find(...)`. We recommend to create new |
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.
In LB 3.x, the REST API for relations was exposed via static methods with the name following the pattern __{methodName}__{relationName}__
, for example Customer.__find__orders
.
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.
Oh I was looking at https://loopback.io/doc/en/lb3/HasMany-relations.html#methods-added-to-the-model when I wrote that. But you're right, this is about REST APIs 👍
docs/site/HasMany-relation.md
Outdated
for them. | ||
|
||
{% include note.html content=" | ||
The type of `orderData` above will possibly change to `Partial<Order>` exclude |
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 you are missing "to" before "exclude":
change to `Partial<Order>` to exclude
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 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.
Good stuff
Can you ping Diana for help so that you can match your diagram with styling in hers (the one for Todo)?
docs/site/Relations.md
Outdated
of the model instances and query and filter the information based on the | ||
client’s needs. | ||
|
||
Model relation in LoopBack 3 is one of its powerful features which help users |
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.
helps
docs/site/Relations.md
Outdated
each of the models, and add querying and filtering capabilities for the relation | ||
APIs after scaffolding their LoopBack applications. In LoopBack 4, with the | ||
introduction of [repositories](Repositories.md), we aim to simplify the approach | ||
to relations by creating constrained repositories. This means that, based on 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.
No comma here
docs/site/Relations.md
Outdated
introduction of [repositories](Repositories.md), we aim to simplify the approach | ||
to relations by creating constrained repositories. This means that, based on the | ||
relation definition, certain constraints need to be honoured by the target model | ||
repository, and thus, we produce a constrained version of it as a navigational |
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.
no comma: repository, and thus we produce ....
|
||
- [HasMany](HasMany-relation.md) | ||
|
||
The articles on each type of relation above will show you how to leverage 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.
I wonder if we should provide an example of what the controller route of a related model would look like here. I think it'd provide users with good idea of what relation
involves in code
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.
Well we provide all that information at the hasMany
relation page, so wouldn't that be sufficient?
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.
Sure, but I think that readers shouldn't be expected to find an example of relation
in general being used in the pages dedicated to each of the relations.
That being said, probably not a big deal
docs/site/HasMany-relation.md
Outdated
key on the source model. This relation indicates that each instance of the | ||
declaring or source model has zero or more instances of the target model. For | ||
example, in an application with customers and orders, a customer can have many | ||
orders, as illustrated in the diagram 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.
no comma
docs/site/HasMany-relation.md
Outdated
|
||
- `create` for creating a target model instance belonging to customer model | ||
instance | ||
([API Docs](https://apidocs.strongloop.com/@loopback%2fdocs/repository.html#DefaultHasManyEntityCrudRepository.prototype.create)) |
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 noticed that the apidocs for HasManyInterface
is more descriptive. Should we replace the link with that anchor or nah
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.
Good catch. Yes, will replace 👍
docs/site/HasMany-relation.md
Outdated
|
||
```ts | ||
import {post, param, requestBody} from '@loopback/rest'; | ||
import {customerRepository} from '../repositories/customer.repository.ts'; |
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.
nitpick: ../repositories
docs/site/HasMany-relation.md
Outdated
``` | ||
|
||
In LoopBack 3, the REST APIs for relations were exposed using static methods | ||
with the name following the pattern `__{methodName}__{relationName}__`, for |
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.
wrap the for example ...
in parenthesis instead of using a comma break.
docs/site/HasMany-relation.md
Outdated
|
||
In LoopBack 3, the REST APIs for relations were exposed using static methods | ||
with the name following the pattern `__{methodName}__{relationName}__`, for | ||
example `Customer.__find__orders`. We recommend to create a new controller for |
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.
Line break here please. The latter paragraph might be better as a note, but not a big deal
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 think a line break is needed here since the first sentence is supposed to be an opener for the whole paragraph.
docs/site/HasMany-relation.md
Outdated
Customer, | ||
typeof Customer.prototype.id | ||
> { | ||
public orders: HasManyRepositoryFactory<Order, typeof Customer.prototype.id>; |
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 generics here need to be switched around.
06fa639
to
411d3a9
Compare
docs/site/HasMany-relation.md
Outdated
|
||
![hasMany relation illustration](./imgs/hasMany-relation-example.png) | ||
|
||
The target model, **Order**, has a property, **customerId**, as the foreign key | ||
to reference the declaring model **Customer's** primary key **id**. | ||
The diagram shows target model **Order** has a property **customerId** as 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.
Missed this in the last review, remove the a
form has a property
docs/site/HasMany-relation.md
Outdated
relational repositories and thus the controllers which use them. Therefore, as | ||
shown above, don't add order-related methods to `CustomerController`, but | ||
instead create a new `CustomerOrdersController` class for them. | ||
with the name following the pattern `__{methodName}__{relationName}__` (for e.g. |
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.
e.g. stand for for example
(don't ask me why, probably something to do with latin). Right now this reads for for example
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 is short for the Latin exempli gratia, “for the sake of example.”
👍
|
||
- [HasMany](HasMany-relation.md) | ||
|
||
The articles on each type of relation above will show you how to leverage 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.
Sure, but I think that readers shouldn't be expected to find an example of relation
in general being used in the pages dedicated to each of the relations.
That being said, probably not a big deal
@shimks I've applied your feedback; LGTY now (we can revisit adding example to relation page later)? |
Documentation for
has many
relation. Includes sections on how to define and configurehas many
relation, and use its CRUD APIs in controller methods.Connect to #1351
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated