Skip to content
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: migrate custom model methods #4476

Merged
merged 1 commit into from
Jan 28, 2020
Merged

Conversation

emonddr
Copy link
Contributor

@emonddr emonddr commented Jan 21, 2020

Content for docs/site/migration/models/methods.md

Explains how to migrate model methods defined by the user.

  • Methods related to persistence go to model-specific Repository class
  • Remote methods (public API) go to Controller class

connect to: #3949

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@emonddr emonddr self-assigned this Jan 21, 2020
@emonddr emonddr force-pushed the dremond_migrate_model_methods branch 6 times, most recently from 8ff376f to 06aa2eb Compare January 22, 2020 20:59
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for kicking off this work!

I like that you are mentioning disableRemoteMethodByName API and the whole idea of hiding some of the standard REST API endpoints, it used to be a frequently asked feature in the past.

I see the following scenarios that should be covered by our docs:

  • At Repository level:
    • Customize behavior of a method
    • Add a new method (e.g. findByName in addition to built-in findById and find)
  • At Controller level:
    • Remove an endpoint
    • Customize the behavior of an existing (scaffolded) endpoint
    • Change the signature (API spec) of an existing (scaffolded) endpoint
    • Add a new (custom) endpoint

I think that some of those scenarios are not specific to migration from LB3 to LB4 and thus can be covered elsewhere. (E.g. adding a new method to a Repository or a Controller.) In the context of the migration guide, I consider as important to connect the dots and explain things from the point of view of a LB3 user.

  • If my LB3 model has a custom method that's exposed via REST API, then I am supposed to implement two new methods in LB4 - one in the repository and another in the controller.
  • If my LB3 model customizes behavior of a built-in method while preserving the (public) API (both REST and JavaScript), then in LB4 I need to change the Repository method only.
  • If my LB3 model changes only the public (REST) API of a built-in method, then in LB4 it's probably enough to customize the Controller method while keeping the default Repository implementation.

```

It extends [DefaultCrudRepository](https://github.com/strongloop/loopback-next/blob/master/packages/repository/src/repositories/legacy-juggler-bridge.ts). To customize the `find` method, you would override this method in the `NoteRepository` class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a code snippet showing how a custom version of find would look like? Ideally, the snippet should show the outcome of migrating the custom method described earlier in LB3 code snippet.


To hide all REST API endpoints of a particular model, simply do not create a controller class for it.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain how to customize a particular REST API endpoint? How to add a new (custom) endpoint?

})
async deleteById(@param.path.number('id') id: number): Promise<void> {
await this.noteRepository.deleteById(id);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this code snippet as way too long, hiding more important content on this page.

I am also concerned about maintenance costs, I am pretty much certain we will often forget to update this code after modifying controller templates in our CLI.

Can you please shorten this snippet and show only one or two methods? You can point users to one of our example controllers (e.g. https://github.com/strongloop/loopback-next/blob/master/examples/todo-list/src/controllers/todo-list.controller.ts) for a full version.

@emonddr emonddr force-pushed the dremond_migrate_model_methods branch 2 times, most recently from 3879a02 to fcd43cb Compare January 24, 2020 21:39
@emonddr
Copy link
Contributor Author

emonddr commented Jan 24, 2020

After speaking with @bajtos , we decided:

to remove this :

If my LB3 model changes only the public (REST) API of a built-in method, then in LB4 it's probably enough to customize the Controller method while keeping the default Repository implementation.

to leave this discussion

At Repository level:
Customize behavior of a method
Add a new method (e.g. findByName in addition to built-in findById and find)
At Controller level:
Remove an endpoint
Customize the behavior of an existing (scaffolded) endpoint
Change the signature (API spec) of an existing (scaffolded) endpoint
Add a new (custom) endpoint

for a non-migration guide.

This document will focus on :

a) how to control which endpoints become public

b) If my LB3 model has a custom method that's exposed via REST API, then I am supposed to implement two new methods in LB4 - one in the repository and another in the controller.

c) If my LB3 model customizes behavior of a built-in method while preserving the (public) API (both REST and JavaScript), then in LB4 I need to change the Repository method only.

@emonddr emonddr force-pushed the dremond_migrate_model_methods branch from fcd43cb to 6bcfece Compare January 24, 2020 22:12
@emonddr emonddr changed the title [WIP] docs: migrate custom model methods docs: migrate custom model methods Jan 24, 2020
@emonddr emonddr marked this pull request as ready for review January 24, 2020 22:14
@emonddr emonddr requested a review from raymondfeng as a code owner January 24, 2020 22:14
@emonddr emonddr force-pushed the dremond_migrate_model_methods branch from 6bcfece to 5053d44 Compare January 27, 2020 14:11
Copy link
Contributor

@hacksparrow hacksparrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely writeup! Left some minor comments.

@emonddr emonddr force-pushed the dremond_migrate_model_methods branch from 5053d44 to 917d108 Compare January 28, 2020 14:59
@emonddr
Copy link
Contributor Author

emonddr commented Jan 28, 2020

@slnode test please

@emonddr emonddr force-pushed the dremond_migrate_model_methods branch from 917d108 to e958761 Compare January 28, 2020 15:30
`findByTitle` to `NoteRepository`, a **new** method `findByTitle` to
`NoteController`, and specify an endpoint url of `/notes/byTitle/{title}`.

**src/repositories/note.repository.ts**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update to use {% include code-caption.html content="..." %} here and similar areas?

@emonddr emonddr force-pushed the dremond_migrate_model_methods branch from e958761 to 3074ef3 Compare January 28, 2020 18:46
@emonddr emonddr merged commit c5d54f6 into master Jan 28, 2020
@emonddr emonddr deleted the dremond_migrate_model_methods branch January 28, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants