-
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
Add TodoList package/tutorial: Todo application with HasMany relation #1518
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.
I mainly want to get feedback on the list of APIs given here. Should we offer more than the very basic CRUD methods? Should we separate relation based TodoList routes
routes from the non-relation based ones?
Another thing to decide on with the PR is how we're going to be reducing code maintenance with the creation of this new PR; there are going to be overlaps between the new package and the existing example-todo
.
An option I see is to have @loopback-examples-todo
as a dependency in this new package and when building, have the todo files cloned from node_modules and then have native TodoList files overwrite the ones that have been cloned.
I'm also not set on this package's name. It might be between todo-intermediate
and todo-list
. The problem with the former is that it's not very descriptive and the problem with the latter is that it can be easily confused with the existing todo
package.
Any suggestions are welcome :p.
|
||
@del('/todo-lists/{id}') | ||
async deleteTodo(@param.path.number('id') id: number) { | ||
return await this.todoListRepo.deleteById(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 wonder when deleting a TodoList whether we should be deleting its Todos as well. With the context of the app being created here, this makes sense to me. Any thoughts @strongloop/sq-lb-apex?
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 when deleting a ToDoList, it makes sense to delete the ToDos inside the list as well.
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 a typical HasMany/BelongsTo relation, the SQL database is configured with a foreign key constraint to ensure each Todo belongs to an existing TodoList. As a result, you cannot delete a TodoList before all owned Todos are deleted first, because otherwise the foreign key constraint would be violated.
Better ORMs support cascading deletes, where the SQL database is configured to automatically delete all owned Todo items when a TodoList is going to be deleted. LoopBack's autoupdate/automigrate does not support that functionality yet and NoSQL databases don't implement cascading deletes, therefore I think it's best to explicitly delete all linked Todos before deleting the TodoList they belong to.
IMO, the implementation of cascading delete DOES NOT belong to controllers, it should be implemented at the repository level.
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.
Has this issue been tracked anywhere? If not, I'd like to track it and have it be separated from this issue.
I'd also like to argue that this should be implemented in the upcoming juggler instead of the repository to keep the SQL/noSQL cases separate from our repositories
"name": "TodoListDb", | ||
"connector": "memory", | ||
"localStorage": "", | ||
"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.
I'll probably end up persisting the memory db with some examples 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.
Is there any particular reason for using a different datasource for Todo and TodoList models? IMO, most applications will use the same datasource (think of a MySQL database or MongoDB collection) to store data of all models.
I am proposing to keep the todo-intermediate
example simple and use the same db
datasource for both Todo
and TodoList
.
I haven't reviewed the code yet but at a high level instead of copying files from This would validate the idea of a |
@virkt25 While I like the idea and was initially trying to see if that would work, I wrote it off since it would only be able to use artifacts that are bindable through the context system. This means if we want to use or reference the I'll try to see if I can make it work by having the existing todo package export its artifacts for type definitions. |
Based on #953 not being completely fleshed out yet along with the fact that a component is only able to register controllers, servers, and providers atm, I don't think we can go with the component idea. If we were to go with the component route, we'd require users to manually bind repositories registered in the component which is something we should avoid in a tutorial. To leave aside the extensibility part out of the equation, I think it's best to approach the tutorial from picking up where |
Not sure what you mean by "limits what your datasources can be named? Repositories packages in a component are actually automatically mounted if an Application is using the RepositoryMixin. I don't think there's any serious limitations to using a Component for this and it would be good to identify any major short comings. |
Looking at the current code in I didn't know about 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 mainly want to get feedback on the list of APIs given here. Should we offer more than the very basic CRUD methods?
Let's keep this example simple please. IMO, the current API (find/create/delete) is sufficient. We want to show how to setup HasMany relation, it's not our goal to build a fully usable Todo API.
Should we separate relation based TodoList routes routes from the non-relation based ones?
Yes please, see my comment below.
Another thing to decide on with the PR is how we're going to be reducing code maintenance with the creation of this new PR; there are going to be overlaps between the new package and the existing example-todo.
I would not worry about overlaps and instead make it easy for each example to use slightly different domain model. For example, to demonstrate relations, we don't need any GeoCoding service as it only distracts the readers and slows down the test suite.
An option I see is to have @loopback-examples-todo as a dependency in this new package and when building, have the todo files cloned from node_modules and then have native TodoList files overwrite the ones that have been cloned.
I agree it would be great if LB4 made it easy to share models & repositories via extensions. That's way beyond the scope of DP3 though. Let's keep this pull request simple please.
I'm also not set on this package's name. It might be between todo-intermediate and todo-list. The problem with the former is that it's not very descriptive and the problem with the latter is that it can be easily confused with the existing todo package.
How about something like grouped-todos
or todos-in-lists
? Maybe it's enough to change your proposed name into plural, i.e. todo-lists
, to make it clear that the example is showing how to have multiple lists of todo items.
|
||
@del('/todo-lists/{id}') | ||
async deleteTodo(@param.path.number('id') id: number) { | ||
return await this.todoListRepo.deleteById(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.
In a typical HasMany/BelongsTo relation, the SQL database is configured with a foreign key constraint to ensure each Todo belongs to an existing TodoList. As a result, you cannot delete a TodoList before all owned Todos are deleted first, because otherwise the foreign key constraint would be violated.
Better ORMs support cascading deletes, where the SQL database is configured to automatically delete all owned Todo items when a TodoList is going to be deleted. LoopBack's autoupdate/automigrate does not support that functionality yet and NoSQL databases don't implement cascading deletes, therefore I think it's best to explicitly delete all linked Todos before deleting the TodoList they belong to.
IMO, the implementation of cascading delete DOES NOT belong to controllers, it should be implemented at the repository level.
} | ||
|
||
@get('/todo-lists') | ||
async findTodos() { |
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.
findTodoLists
?
TBH, I don't see much value in repeating the model name in every controller method - the model name is already captured in the class name. The current version of the REST Controller template used by lb4 controller
seems to follow this convention too.
Could you please create the core of this new controller by calling lb4 controller
instead of modifying code copied from TodoController
?
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 that case, I think it makes sense to also modify Controller methods in Todo
to be built from lb4 controller (CRUD)
command, maybe in a separate PR. Thoughts?
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 that case, I think it makes sense to also modify Controller methods in
Todo
to be built fromlb4 controller (CRUD)
command, maybe in a separate PR. Thoughts?
That would be great 👍
} | ||
|
||
@post('/todo-lists/{id}/todos') | ||
async createTodoForTodoList( |
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 move methods implementing REST API for the relation "TodoList hasMany Todo" into a new controller TodoListTodosController
(feel free to pick a better name). See #1500 (comment)
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.
"name": "TodoListDb", | ||
"connector": "memory", | ||
"localStorage": "", | ||
"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.
Is there any particular reason for using a different datasource for Todo and TodoList models? IMO, most applications will use the same datasource (think of a MySQL database or MongoDB collection) to store data of all models.
I am proposing to keep the todo-intermediate
example simple and use the same db
datasource for both Todo
and TodoList
.
See [Conventional Commits](https://conventionalcommits.org) for commit guidelines. | ||
|
||
<a name="0.12.5"></a> | ||
## [0.12.5](https://github.com/strongloop/loopback-next/compare/@loopback/[email protected]...@loopback/[email protected]) (2018-06-28) |
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 remove the CHANGELOG, todo-intermediate should start with clean history IMO.
"pretest": "npm run build:current", | ||
"test": "lb-mocha \"DIST/test/*/**/*.js\"", | ||
"test:dev": "lb-mocha --allow-console-logs DIST/test/**/*.js && npm run posttest", | ||
"verify": "npm pack && tar xf loopback-todo*.tgz && tree package && npm run clean", |
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.
You will need to change this to loopback-example-todo-intermediate*.tgz
.
"build:dist8": "lb-tsc es2017", | ||
"build:dist10": "lb-tsc es2018", | ||
"build:watch": "lb-tsc --watch", | ||
"clean": "lb-clean *example-todo*.tgz dist* package api-docs", |
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.
*example-todo-intermediate*.tgz
} | ||
|
||
setupServices() { | ||
this.service(GeocoderServiceProvider); |
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.
Since the primary goal of todo-intermediate
is to show model relations, I am proposing to simplify the Todo
model and remove geo lookup:
- There will be less moving parts to maintain.
- Smaller codebase will make it easier for LB4 user to understand what are the relevant bits to learn from this example
- The geocoder service slows down the test suite, we don't want to have twice as many slow tests as we already have.
@property({ | ||
type: 'string', | ||
}) | ||
remindAtAddress: string; // address,city,zipcode |
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.
Let's remove remindAtAddress
and remindAtGeo
properties, they are not relevant to 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.
Hopefully this the last commit with any major changes to the API of the example.
Please don't review small stuff like licensing header or package.json; it'll be coming once the API/tests are finalized.
expect(response.body).to.containDeep(todoLists); | ||
}); | ||
|
||
it.skip('updates all todoLists', async () => { |
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.
Skipped for now since there's a bug (I think) with updateAll
atm where an error is thrown when updateAll(undefined, someData)
signature is used. See #1529
.and.not.containEql(notMyTodo.toJSON()); // is this assertion necessary? | ||
}); | ||
|
||
it.skip('updates todos for a todoList', async () => { |
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 ^^^
before(() => { | ||
client = createClientForHandler(app.requestHandler); | ||
}); | ||
|
||
// TODO: refactor into multiple files | ||
beforeEach(async () => { | ||
await todoRepo.deleteAll(); |
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.
TODO: split this test file into two:
- todo.acceptance.ts
- todo-list.acceptance.ts
|
||
expect(response.body) | ||
.to.containDeep(myTodos) | ||
.and.not.containEql(notMyTodo.toJSON()); // is this assertion necessary? |
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 Do you feel it is up to the users to test whether the queries have been properly constrained here?
This line for example, I assert that the models discovered by the constrained repository's find
does not contain a todo with a different foreign key that the repository's been constrained with.
Is this assertion unnecessary since it's something that the users should trust with the framework?
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 acceptance tests, I'd prefer to verify that the application works correctly end-to-end, i.e. verify assumptions made about the framework too.
On the other hand, if we had an integration test to verify that our HasMany repository correctly filters the records, and a unit test with mocked repositories to verify that the controller calls the correct HasMany repository method, then this assertion may not be needed.
Let's keep the assertion for now.
async createTodoList() { | ||
const newTodoList = {createdAt: new Date()}; | ||
return await this.todoListRepo.create(newTodoList); | ||
async create(@requestBody() obj: TodoList): Promise<TodoList> { |
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.
Note, the controller has been refactored with the one that can be generated through the CLI
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.
Should we preserve the feature that sets createdAt
to the current date?
I think this feature should be implemented at model level via defaultFn: 'now'
setting.
@property({name: 'createdAt', type: Date, defaultFn: 'now'});
|
||
@patch('/todo-lists/{id}/todos') | ||
async patch(@param.path.number('id') id: number, @requestBody() todo: Todo) { | ||
return await this.todoListRepo.todos(id).patch(todo); |
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 little bit confused about this endpoint, it is always patching ALL todos belonging to the given todoListId
? That don't seem like a very useful feature to me. Are we perhaps missing a where
parameter?
async createTodoList() { | ||
const newTodoList = {createdAt: new Date()}; | ||
return await this.todoListRepo.create(newTodoList); | ||
async create(@requestBody() obj: TodoList): Promise<TodoList> { |
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.
Should we preserve the feature that sets createdAt
to the current date?
I think this feature should be implemented at model level via defaultFn: 'now'
setting.
@property({name: 'createdAt', type: Date, defaultFn: 'now'});
@property({required: true}) | ||
title: string; | ||
@property({required: true}) | ||
lastModified: 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.
Hmm, do we have an easy way how to enforce lastModified
to be always accurately set? It must not be a responsibility of clients to keep this field updated whenever any data changes!
I don't think juggler supports such feature out of the box, one needs to use before save
or persist
hook, which is not documented for LB4 models/repositories (yet).
I am proposing to remove this property. If we need another property besides id and title, then I am proposing to define a property called color
(inspired by Apple's Reminders app).
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'm hoping this is the last commit to define the physical files that are going to be going into the package. I'll be working on the tutorial steps themselves tomorrow based on what I have here.
client = createClientForHandler(app.requestHandler); | ||
}); | ||
|
||
// TODO: refactor into multiple files |
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 this TODO entry still relevant?
let deleteAll: sinon.SinonStub; | ||
let findById: sinon.SinonStub; | ||
let updateById: sinon.SinonStub; | ||
let deleteById: sinon.SinonStub; |
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.
These stubs seem to be repeated in multiple test files. Is it time to refactor/extract out a shared helper for setting up a set of stubs for CrudRepository?
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.
Considering that these stub references are just references and the actually setting up of the stubs is done by a single line (createStubInstance(ctor)
), I think we can just refactor the code to avoid defining these 'shortcut' references and just use direct references to the stub methods in our tests. Thoughts?
a74afda
to
6f4c3ef
Compare
@@ -57,6 +57,23 @@ children: | |||
url: todo-tutorial-geocoding-service.html | |||
output: 'web, pdf' | |||
|
|||
- title: 'TodoList Tutorial' | |||
url: todo-list-tutorial.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 link currently broken atm. It can't be fixed until a script on loopback.io is updated. I'll wait until everything is finalized (e.g. the package name for the new example) before I submit a PR over there.
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's the issue with loopback.io
?
examples/todo-list/README.md
Outdated
This tutorial demonstrates how to create a set of APIs for models that are | ||
related to another. | ||
|
||
![todo-tutorial-overview](REVISED VERSION OF THE DIAGRAM) |
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 revised version of the diagram is not complete yet :p
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.
General linting stuff has still not been done, so please ignore them if you see any obvious problems :p
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.
@shimks Great effort! I reviewed the doc part, it's quite straightforward for me to follow, the logic and how you break it down into smaller tutorials is reasonable.
Just a few nitpicks :)
Will try out the code when take a second review.
examples/todo-list/README.md
Outdated
@@ -27,23 +27,30 @@ Lastly, you'll need to install the LoopBack 4 CLI toolkit: | |||
npm i -g @loopback/cli | |||
``` | |||
|
|||
If you're not following from the | |||
[todo tutorial](http://loopback.io/doc/en/lb4/todo-tutorial.html), you can use | |||
the LoopBack 4 CLI tool to catch up to where this tutorial will continue from: |
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.
@shimks Hmm...I find a little hard to understand the guide here:
Do you mean people should follow the todo
tutorial then continue with this one?
that it can be used to measure the progress of a bigger picture that the item is | ||
a part of. | ||
|
||
We'll create TodoList model to represent a checklist that contains multiple Todo |
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: TodoList --> TodoList
permalink: /doc/en/lb4/todo-list-tutorial-model.html | ||
summary: LoopBack 4 TodoList Application Tutorial - Add TodoList 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.
nitpick: how about link to it's previous tutorial at the beginning?
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
} | ||
``` | ||
|
||
Don't forget to decorate your new relations property with `@hasMany`! 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.
nitpick: @hasMany
--> @hasMany()
permalink: /doc/en/lb4/todo-list-tutorial-repository.html | ||
summary: LoopBack 4 TodoList Application Tutorial - Add TodoList Repository | ||
--- | ||
|
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, it would be good to add a link to previous step :)
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 at the bottom of the page -- similar to todo-tutorial
.
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.
Awesome job!! I haven't reviewed the code / nitpicked stuff but did go over the tutorial in detail and leaving a few comments to help improve it. Can't wait to see this land!
|
||
### Building a checklist for your Todo models | ||
|
||
A todo item is often grouped into a checklist along with other todo items so |
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 heading and paragraph might be a better start to this page ... and then jumping into talking about models with relations
followed by the second paragraph of this section
} | ||
``` | ||
|
||
Don't forget to decorate your new relations property with `@hasMany`! 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.
Instead of saying don't forget ... I think it's better to say something like Notice the
@hasmany() decorator used to define this property, it lets LB4 know that a todo list can have many todo items
.
Reason for not saying don't forget is because this follows a code sample a user will likely copy and paste into their application.
permalink: /doc/en/lb4/todo-list-tutorial-repository.html | ||
summary: LoopBack 4 TodoList Application Tutorial - Add TodoList Repository | ||
--- | ||
|
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 at the bottom of the page -- similar to todo-tutorial
.
|
||
### Repositories with related models | ||
|
||
The repository of a model with relations can make constrained version of 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.
The first line is very confusing and hard to understand IMO for someone reading it for the first time. Also it would be good to explain what exactly is meant by a constrained repository
? (You know it, I know it but someone reading it for the first time will be lost).
- update `index.ts` to export the newly created repository | ||
|
||
Like `TodoRepository`, we'll use `DefaultCrudRepository` to extend our TodoList | ||
repository and inject the same datasource. From there we'll need to make two |
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 explain we're using the same datasource since we want to persist the list on the same database?
@repository(TodoListRepository) protected todoListRepo: TodoListRepository, | ||
) {} | ||
|
||
@post('/todo-lists/{id}/todos') |
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 is why we need #918
|
||
```sh | ||
$ lb4 controller | ||
? Controller class name: TodoListTodo |
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 this the best practice we want to promote? ... a new controller for the relation APIs? If I was writing this application since the base path for this is /todo-lists
, I would add the new controller methods to the same file as TodoListController
instead of TodoListTodoController
.
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 this the best practice we want to promote? ... a new controller for the relation APIs?
Yes, each relation should have its own controller.
While it may be tempting to add relation API methods to the same controller which provide regular CRUD methods, this approach does not scale well as more relations are added to the original model.
Having a new controller for each relation makes it also easier for our CLI to scaffold REST API implementation - we don't need to modify any existing controller code, just generate a new controller file.
decorator's name suggests, this will let LoopBack 4 know that a todo list can | ||
have many todo items. | ||
|
||
To complement `TodoList`'s relationship to `Todo`, we'll add in `todoListId` |
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 this necessary? ... There's no reverse relationship such as belongsTo
yet so not sure why we want to add this property to the Todo
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.
Yes IMO as todoListId
is the foreign key on the Todo
model which references the id
of the TodoList
model and is needed for the hasMany
relation.
@@ -57,6 +57,23 @@ children: | |||
url: todo-tutorial-geocoding-service.html | |||
output: 'web, pdf' | |||
|
|||
- title: 'TodoList Tutorial' | |||
url: todo-list-tutorial.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.
What's the issue with loopback.io
?
@@ -112,6 +112,13 @@ including SOAP or REST services. Continue to the bonus section to learn how | |||
LoopBack connectors make it super easy to fetch data from other services and | |||
[enhance your Todo application with location-based reminders](todo-tutorial-geocoding-service.md). | |||
|
|||
### The next step: TodoList Application | |||
|
|||
If you would like to try out using some of the more advanced features 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.
We also have a bonus step below ... this part seems a bit disjointed. How about making a section called Next Steps
and then saying to try out the to learn about adding an external service for geolocation to the Todo App. Or try out the todolist
tutorial to learn about relations in LoopBack 4.
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.
❗️Important: please remove examples/todo-list/CHANGELOG.md
.
I am afraid I don't have bandwidth to review this pull request in detail. I'll leave that up to other team members. Sorry!
|
||
```sh | ||
$ lb4 controller | ||
? Controller class name: TodoListTodo |
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 this the best practice we want to promote? ... a new controller for the relation APIs?
Yes, each relation should have its own controller.
While it may be tempting to add relation API methods to the same controller which provide regular CRUD methods, this approach does not scale well as more relations are added to the original model.
Having a new controller for each relation makes it also easier for our CLI to scaffold REST API implementation - we don't need to modify any existing controller code, just generate a new controller file.
examples/todo-list/LICENSE
Outdated
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2017,2018. All Rights Reserved. | |||
Node module: @loopback/example-todo |
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.
Node module: @loopback/example-todo-list
examples/todo-list/package.json
Outdated
"build:dist8": "lb-tsc es2017", | ||
"build:dist10": "lb-tsc es2018", | ||
"build:watch": "lb-tsc --watch", | ||
"clean": "lb-clean *example-todo*.tgz dist* package api-docs", |
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.
*example-todo-list*
examples/todo-list/package.json
Outdated
"pretest": "npm run build:current", | ||
"test": "lb-mocha \"DIST/test/*/**/*.js\"", | ||
"test:dev": "lb-mocha --allow-console-logs DIST/test/**/*.js && npm run posttest", | ||
"verify": "npm pack && tar xf loopback-todo*.tgz && tree package && npm run clean", |
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.
loopback-example-todo-list*.tgz
|
||
### Create TodoList controller | ||
|
||
Starting with creating a controller for our `TodoList` routes, run the CLI |
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 suggest to say Run the CLI command for creating a RESTful CRUD controller for our 'TodoList' routes with the following inputs:
or start with Let's run
/Let's start by running
for the same sentence.
decorator's name suggests, this will let LoopBack 4 know that a todo list can | ||
have many todo items. | ||
|
||
To complement `TodoList`'s relationship to `Todo`, we'll add in `todoListId` |
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 IMO as todoListId
is the foreign key on the Todo
model which references the id
of the TodoList
model and is needed for the hasMany
relation.
`TodoRepository` | ||
- inject `TodoRepository` instance | ||
|
||
Once the call signature of `todos` have been defined, use |
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 we're talking about HasManyRepositoryFactory<Todo, typeof TodoList.prototype.id>
we can say type?
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.
awesome work so far!
docs/site/Examples-and-tutorials.md
Outdated
- **[todo](todo-tutorial.md)**: Tutorial on building a simple application with | ||
LoopBack 4 key concepts using bottom-up approach. | ||
|
||
- **[todo-list](todo-list-tutorial.md)**: Tutorial on introducing related models |
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.
Tutorial on introducing models relations and relation CRUD APIs by building upon the Todo tutorial.
docs/site/Examples-and-tutorials.md
Outdated
- **[todo-list](todo-list-tutorial.md)**: Tutorial on introducing related models | ||
and building their API from the Todo tutorial | ||
|
||
- **[hello-world](https://github.com/strongloop/loopback-next/tree/master/examples/hello-world)**: |
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.
Why is hello-world
moved below the more advanced tutorial? Order should be hello-world -> todo -> todo-list
### Controllers with related models | ||
|
||
Defining business logic to handle requests to related models isn't too different | ||
from handling requests to standalone models. We'll create controllers to handle |
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 standalone models
or for standalone models
? -- go with what makes sense to you.
} | ||
``` | ||
|
||
Once the routes for the other HTTP verbs have been added, the controller should |
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 tutorial doesn't mention to a user how / what other HTTP verbs they can / should define. Shouldn't we be guiding the user here? -- perhaps something like Now we'll add other HTTP verbs such as GET, PUT, DELETE and the completed controller should look as follows:
### Try it out | ||
|
||
With the controllers complete, your application is ready to start up again! The | ||
boot module we configured back 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.
Don't think there's any configuration needed for boot
... so mentioning that can be a bit confusing. I'd just leave it at starting the application?
examples/todo-list/README.md
Outdated
|
||
## Tutorial | ||
|
||
To follow this tutorial, begin with 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.
instead of begin with the ... section
it might be better to say, let's begin by [Adding a TodoList model]`.
rpc-server: A basic RPC server using a made-up protocol. | ||
``` | ||
|
||
2. Jump into the directory and then install the required dependencies: |
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 don't install dependencies?
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.
Nope, never really questioned it but we probably should
examples/todo-list/README.md
Outdated
``` | ||
|
||
Feel free to look around in the application's code to get a feel for how it | ||
works, or if you're still interested in learning how to build it step-by-step, |
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 do you mean by still interested
? ...
Feel free to look around in the application's code to get a feel for how it works, or build it step-by-step by following this tutorial!
examples/todo-list/README.md
Outdated
|
||
### Stuck? | ||
|
||
Check out our [Gitter channel](https://gitter.im/strongloop/loopback) and ask |
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.
Do we really want to promote Gitter
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.
idk tbh, I'm just going to remove it
examples/todo-list/package.json
Outdated
"loopback", | ||
"LoopBack", | ||
"example", | ||
"tutorial" |
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.
relations, crud, models, todo ... ?
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 (and hasmany
, but may be redundant)
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.
Great work!
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!
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.
Just some minor nitpicks.
|
||
Like `TodoRepository`, we'll use `DefaultCrudRepository` to extend our | ||
`TodoListRepository`. We'll be using the same database used for | ||
`TodoRepository`, we'll inject `datasources.db` in this repository as well. From |
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.
so we'll
? ... it's just weird to read we'll 3 times in the same sentence.
}); | ||
|
||
it('updates todos for a todoList', async () => { | ||
const todoList = await givenTodoListInstance(); |
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 a follow up PR it would be good to move this to a beforeEach
since it seems repeated in each file. All acceptance tests seem to have a similar repeated line of code at the start of each test.
related to #1358
This PR is intended to create a separate package from
example-todo
which would incorporate using relations, potentially along with other features coming to loopback-next.The first commit merely copies over files from Todo with no meaningful changes made in between.
Please review from 2nd commits and onward.
Things not included in the PR (yet):
testsinstructions on creating the appentry for CLI example prompt(can't really test this unfortunately since the CLI attempts to download from master)new diagramgeneral linting/polishFollow up/concurrent PRs to come:
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated