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

Migration guide for model relations #4058

Merged
merged 2 commits into from
Nov 14, 2019
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Nov 7, 2019

Describe how to migrate model relations from LB3 to LB4.

Fix #3948

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 👈

@bajtos bajtos added this to the Nov 2019 milestone milestone Nov 7, 2019
@bajtos bajtos self-assigned this Nov 7, 2019
@bajtos bajtos force-pushed the docs/migrate-model-relations branch from 1e71e11 to 9c0f712 Compare November 12, 2019 09:11
@bajtos bajtos marked this pull request as ready for review November 12, 2019 09:13
@bajtos bajtos requested review from a team November 12, 2019 09:13
@bajtos
Copy link
Member Author

bajtos commented Nov 12, 2019

The pull request is ready for review, @raymondfeng @strongloop/loopback-maintainers PTAL.

I found two more errors I'd like to fix outside of this PR, they are not blocking the guide.

  1. lb4 repository fails when src/repositories directory does not exist (yet) -- see fix(cli): handle missing target artifact dir #4112

  2. The relation controller scaffolded by lb4 relation allows FK property (e.g. categoryId) to be included when creating related models (e.g. POST /categories/{categoryId}/products).

    UPDATE: Never mind, I see that the FK property is marked as optional. It looks like an intentional decision, in which case I am fine with that.

@bajtos bajtos changed the title Migration guide for model relations [WIP] Migration guide for model relations Nov 12, 2019
Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , just some nitpicks

docs/site/migration/models/relations.md Outdated Show resolved Hide resolved
docs/site/migration/models/relations.md Outdated Show resolved Hide resolved
Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

Great stuff

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.

A few nitpicks : ) LGTM

docs/site/migration/models/relations.md Show resolved Hide resolved
[Migrating model definitions and built-in APIs](./core.md), especially
creation of Repository classes for models on both sides of the relation.

2. Run `lb4 relation` to define the model relation in your model class, generate
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make it more clear like:

Run `lb4 relation` to create the model relations according to the relation definitions defined in lb3 model json files.

so that people understand where to find the relation definitions.
And we can link to the generator doc as well: https://loopback.io/doc/en/lb4/Relation-generator.html

Copy link
Member Author

Choose a reason for hiding this comment

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

according to the relation definitions defined in lb3 model json files.

Copy link
Member Author

@bajtos bajtos Nov 14, 2019

Choose a reason for hiding this comment

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

according to the relation definitions defined in lb3 model json files.

I am concerned that this is not entirely correct. Relations can be defined from JS code too, e.g. Product.belongsTo(Category).

I am not sure how to best incorporate this information into the migration guide. Let's leave it out of scope of this pull request. If you have some ideas how to improve the current text, then can you please open a follow-up pull request yourself? 

And we can link to the generator doc as well: https://loopback.io/doc/en/lb4/Relation-generator.html

Great idea 👍

Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

👍

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@bajtos bajtos force-pushed the docs/migrate-model-relations branch from 9c0f712 to 12179b5 Compare November 14, 2019 08:59
@bajtos bajtos merged commit 0f96f2b into master Nov 14, 2019
@bajtos bajtos deleted the docs/migrate-model-relations branch November 14, 2019 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to migrate model relations
5 participants