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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: update model decorator #3354

Merged
merged 1 commit into from Jul 26, 2019

Conversation

@agnes512
Copy link
Contributor

commented Jul 14, 2019

Resolves #2133

Update inconsistent docs for @model and @property decorators part.
By testing out the code in loopback-datasource-juggler/lib/datasource.js, loopback-datasource-juggler/lib/model-builder.js, legacy-juggler-bridge, I made following changes:

@model decorator:

( I copied the available part of the table from lb3 site and made a bit change on the content. Because I think checking sites back and forth is kind of annoying.)

  • indicates that lb4 now only supports settings entries in model decorator. And top-level-properties in lb3 now are passed in settings.
  • indicates that properties are defined via @property decorator.
  • lists features like options, acls are not available in lb4 yet. Not sure why we don't support options. Will open an issue for this part. We already have that issue opened.
  • indicates that lb4 defines relations by relation decorators such as @hasMany.

@property decorator:

Since this part is the same as lb3, I only made slight change.

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 馃憟

@agnes512 agnes512 force-pushed the model-properties-docs branch from e5db215 to 4b75e80 Jul 15, 2019

@agnes512 agnes512 force-pushed the model-properties-docs branch from 4b75e80 to 5919ba1 Jul 22, 2019

@agnes512 agnes512 marked this pull request as ready for review Jul 22, 2019

@agnes512 agnes512 requested review from bajtos and raymondfeng as code owners Jul 22, 2019

@bajtos
Copy link
Member

left a comment

Great initiative!

Let's make sure the documentation matches the actual implementation.

For settings that are documented as supported, it would be great to have tests to verify. Preferably in repository-tests package, so that we can run them against different connectors.

I understand writing such tests may be out of scope of this pull request. I am ok to leave them out as long as you create a follow-up GH issue.

The strict settings is already covered by tests, see packages/repository-tests/src/crud/freeform-properties.suite.ts

docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Show resolved Hide resolved
docs/site/Model.md Show resolved Hide resolved
@bajtos

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@agnes512 great discussion! I think in general, it's important to realize that in LB4, each model has two parallel classes:

  • The class defined by the app developers using decorators and class MyModel extends Entity syntax.
  • The "implementation" class created by legacy-juggler-bridge, this class is inheriting from juggler's Model base class.

Because of this design, not all settings offered by juggler will actually work in LoopBack 4.

You may ask why we have this complexity? The legacy juggler bridge is a (temporary) solution allowing us to implement Repository interfaces using existing juggler & connector codebases. In the future, we would like to roll out a LB4-native implementation of Repository interfaces, this will call connectors directly and won't depend on juggler at all. However, it requires a significant amount of work, thus we decided to use the legacy juggler bridge for now, despite all the complexity it introduces.

BTW I am pretty sure you are not the last person to be caught by surprise about this design, it would be great if you could document what you have learned about this problem domain. For example, we can add DEVELOPING.md file to packages/repository.

@agnes512 agnes512 force-pushed the model-properties-docs branch 3 times, most recently from bdd4af7 to fe6c6ca Jul 23, 2019

@agnes512

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

related issues:

  • discussion field: link
  • options field: link

@agnes512 agnes512 force-pushed the model-properties-docs branch 5 times, most recently from d16e9b7 to ae22a6e Jul 24, 2019

docs/site/Model.md Show resolved Hide resolved
@bajtos

bajtos approved these changes Jul 25, 2019

Copy link
Member

left a comment

Looks mostly good, I have few minor comments to consider.

docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
packages/repository/DEVELOPING.md Outdated Show resolved Hide resolved
packages/repository/DEVELOPING.md Outdated Show resolved Hide resolved

@bajtos bajtos requested a review from jannyHou Jul 25, 2019

@agnes512 agnes512 force-pushed the model-properties-docs branch from ae22a6e to 9a9ac29 Jul 25, 2019

@bajtos bajtos added the Docs label Jul 25, 2019

@agnes512 agnes512 force-pushed the model-properties-docs branch 3 times, most recently from c47da6c to 811015c Jul 25, 2019

@b-admike
Copy link
Member

left a comment

Nice writeup 馃憤

docs/site/Model.md Outdated Show resolved Hide resolved

@agnes512 agnes512 force-pushed the model-properties-docs branch from 811015c to 6f84825 Jul 26, 2019

@nabdelgadir
Copy link
Contributor

left a comment

Great job!

docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
docs/site/Model.md Outdated Show resolved Hide resolved
packages/repository/DEVELOPING.md Outdated Show resolved Hide resolved
packages/repository/DEVELOPING.md Outdated Show resolved Hide resolved
packages/repository/DEVELOPING.md Outdated Show resolved Hide resolved

@agnes512 agnes512 force-pushed the model-properties-docs branch from 6f84825 to 4357c38 Jul 26, 2019

@agnes512 agnes512 force-pushed the model-properties-docs branch from 4357c38 to 478d44b Jul 26, 2019

@agnes512 agnes512 merged commit ee57721 into master Jul 26, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 91.572%
Details

@delete-merged-branch delete-merged-branch bot deleted the model-properties-docs branch Jul 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.