-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: poc #2
base: master
Are you sure you want to change the base?
feat: poc #2
Conversation
1b1f481
to
976334c
Compare
This PR is a PoC of the inclusion handler proposed by @batjos in comment. Takes The PoC contains:
I also discovered a few problems worth discussion:
|
Good catch, can you work with @raymondfeng to improve the type definition please? We should eventually take a look at the JSON Schema/OpenAPI parameter schema emitted for |
typeof Todo.prototype.id, | ||
typeof TodoList.prototype.id | ||
>(todoRepositoryGetter, 'todoListId'); | ||
this._inclusionHandler.todos = todoHandler; |
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 block of code will be repeated a lot, I'd like to see a simpler solution, e.g. a sugar API building on top of InclusionHandlerFactory
.
Ideally, the entire block should collapse to a single statement, e.g.
this._registerInclusion('todos', todoRepositoryGetter);
Under the hood, the helper should look up the definition of todos
relation to learn about the target model, keyTo
, and any other metadata needed. See how _createHasManyRepositoryFactoryFor
is implemented.
Also based on the code in juggler, every relation needs a slightly different implementation of the inclusion handler. Please include an example/acceptance test showing how to include models via belongsTo
relation to ensure requirements of other relation types were considered.
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.
I added the belongsTo handler and it does fetch the parent instance for the child. BUT there is a problem when construct the returned data:
The Todo
model only has property todoListId
as the foreign key, but not todoList
as its parent property. Therefore given the object
const todo = {
id: 1,
title: 'a todo item',
todoListId: 1,
todoList: {
id: 1,
color: red
}
}
method toEntity(todo)
won't convert property todoList
and removes it in the constructed todo entity.
It is important to consider the scenario where we want to fetch multiple source models and include their related models in the result, e.g. |
👌 Sure, will create another PR to fix it. |
Related to loopbackio#1952
Description see my comment below
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated