-
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: update belongsTo docs with decorator syntax #3808
Conversation
6d04c47
to
7af0df9
Compare
closed previous PR #3772 because of travis job errrors. |
docs/site/BelongsTo-relation.md
Outdated
@belongsTo( | ||
() => Customer, | ||
{keyFrom: 'customerId', keyTo: 'id', name: 'customer'}, | ||
{name: 'customer_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.
I think {keyFrom
: '.., name
: ..} is enough for customizing the name ( or even just to have the relation name {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.
this example is to provide all the parameters for the decorator
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.
@deepakrkris When looking at the decorator's implementation, I feel the 3rd parameter(property definition) is more for configuring the property, like:
- Is it required/generated/its lengh/default value, see property configuration,
- And json schema config, see type PropertyDefinition
name
should be inferred from the property cust_Id
itself.
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.
@jannyHou that is very strange. PropertyDefinition does not have a name
key defined, but typescript is accepting the name
attribute ! Also I am explicitly using the name
key to customize db column names and it works perfectly fine for the todo-list 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.
@jannyHou , also one more thing, cust_Id
being a customized name (per standard naming convention it should be customerId
), the name
attribute to indicate target model
cannot be inferred from the customized name, it has to be explicitly stated.
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.
@deepakrkris sorry for replying late (refreshed the PR several times last week but somehow git didn't show your message, weird...)
PropertyDefinition does not have a name key defined, but typescript is accepting the name attribute
It's because type PropertyDefinition
allows arbitrary properties, see code
Also I am explicitly using the name key to customize db column names and it works perfectly fine for the todo-list example
Thank you got it, so it's used to configure the db column name.
724f6ed
to
bf96ded
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.
A minor comment, otherwise LGTM.
1f96124
to
7e84bfe
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.
@deepakrkris Good stuff! Looks good in general, I left a few comments.
docs/site/BelongsTo-relation.md
Outdated
@belongsTo( | ||
() => Customer, | ||
{keyFrom: 'customerId', keyTo: 'id', name: 'customer'}, | ||
{name: 'customer_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.
@deepakrkris When looking at the decorator's implementation, I feel the 3rd parameter(property definition) is more for configuring the property, like:
- Is it required/generated/its lengh/default value, see property configuration,
- And json schema config, see type PropertyDefinition
name
should be inferred from the property cust_Id
itself.
7e84bfe
to
faab9ec
Compare
2db04ae
to
40b8dc6
Compare
40b8dc6
to
8baea38
Compare
6e491fb
to
8bd0c19
Compare
1260549
to
57983c4
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.
LGTM.
My only (irrelevant) concern is that we might want to improve the code base to deal with default values.
constructor and designates the relation type. It also calls `property()` to | ||
ensure that the decorated property is correctly defined. | ||
The standard naming convention for the foreign key property in the source model | ||
is `relation name` + `Id` (for example, Order.customerId). |
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 explanation here looks weird. It seems like that we define the relation name first, then we decide the default value of our foreign key name. This is because keyFrom
defaults to the decorated property name, not targetModel
+ Id
.
I wanted to point out in this PR that, in our code, the way we get the default relation name is by modifying the decorated property name:
customerId -> (get rid of `id`) -> customer
I found this code is really confusing. For example, if we decorate the foreign key name as
@belongsTo(..)
anyRandomNameId
the default relation name will be anyRandomName
, which doesn't make sense at all. I guess it was designed to use the default foreign key customerId
. But the foreign key doesn't default to targetModel
+ Id
. It defaults to the decorated name. That's the reason makes it hard to document. In short, the code doesn't fully deal with default values as we expected.
We might want to improve the way to get the relation name in belongTo
in the future.
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.
yes the code is not very self explanatory in terms of defining relations as of now
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, the FK usually refers to the target model, such as customerId
. anyRandomNameId
does not name the FK meaningfully. In such cases, the options.name should be used.
Fixes #2639
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈