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.

Sign up for GitHub

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

Jump to bottom

docs: add url, diagram for 3 relations for inclusion #4007

Merged
merged 1 commit into from
Oct 28, 2019
Merged

docs: add url, diagram for 3 relations for inclusion #4007

merged 1 commit into from
Oct 28, 2019

Conversation

agnes512
Copy link
Contributor

After triaging some community issues and the inclusion blog, I'd like to enhance our docs for inclusion to have less confusion.

  • add high level diagram to explain the idea and relation of inclusionResolvers and inclusionResolver
  • add graphQL-like diagram to show the idea of how it gets related models.
  • add url GET ... as query examples in case users don't know how to query it with controller/url

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 👈

dhmlau
dhmlau reviewed Oct 25, 2019
View reviewed changes
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

A few minor comments / suggestions. They apply to both BelongsTo and HasMany md files. Thanks.

docs/site/BelongsTo-relation.md Outdated
@@ -328,6 +328,13 @@ on constructor to avoid "Circular dependency" error (see

## Querying related models

LoopBack 4 creates a different inclusion resolver for each relation type. Each
Copy link
Member

Choose a reason for hiding this comment

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

maybe we call it LoopBack instead of LoopBack 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't mention anything about inclusion resolver in LB3. Would it be better if I change it to LB4? or

Different from LB3, LB4 creates a different inclusion resolver for each relation type to query related models.

?

docs/site/BelongsTo-relation.md Outdated
@@ -336,12 +343,18 @@ belongs to a `Customer`.

After setting up the relation in the repository class, the inclusion resolver
allows users to retrieve all orders along with their related customers through
the following code:
the following code in repository level:
Copy link
Member

Choose a reason for hiding this comment

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

in repository level -> in the repository class?

jannyHou
jannyHou approved these changes Oct 28, 2019
View reviewed changes
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

Good diagram. LGTM, just a question about the url.

docs/site/BelongsTo-relation.md
or use APIs with controllers:

```
GET http://localhost:3000/orders?filter[include][][relation]=customer
Copy link
Contributor

Choose a reason for hiding this comment

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

is it an extra []? I am looking at the similar include filter in LB3 https://loopback.io/doc/en/lb2/Include-filter.html#rest-examples:

/customers?filter[include][reviews]=author

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, /customers?filter[include][reviews]=author wouldn't work because include.filter is not a function.

And different queries between LB3 and LB4 make me wonder if we need a querying data page for LB4 as well. Cause right now the only way to check the query syntax is to go to the LB3 site.

Copy link
Contributor

Choose a reason for hiding this comment

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

The include filter for LB2 is different from the LB4 one. In LB4 it's {include: [{relation: 'customer'}]}, whereas in LB2 (from my understanding) it's {include: {customer: [/* ... */]}}.

@agnes512 agnes512 merged commit bc8d46f into master Oct 28, 2019
@agnes512 agnes512 deleted the inclusion-diagram branch October 28, 2019 15:28
@agnes512 agnes512 mentioned this pull request Nov 7, 2019
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhmlau dhmlau dhmlau left review comments

@raymondfeng raymondfeng raymondfeng approved these changes

@jannyHou jannyHou jannyHou approved these changes

@nabdelgadir nabdelgadir nabdelgadir approved these changes

@bajtos bajtos Awaiting requested review from bajtos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agnes512 @raymondfeng @jannyHou @dhmlau @nabdelgadir