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: update model decorator #3354

Merged
merged 1 commit into from
Jul 26, 2019
Merged

docs: update model decorator #3354

merged 1 commit into from
Jul 26, 2019

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 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 Compare July 15, 2019 00:08
@agnes512 agnes512 force-pushed the model-properties-docs branch from 4b75e80 to 5919ba1 Compare July 22, 2019 02:57
@agnes512 agnes512 marked this pull request as ready for review July 22, 2019 03:19
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.

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
Copy link
Member

bajtos 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 Compare July 24, 2019 17:28
@agnes512
Copy link
Contributor Author

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 Compare July 24, 2019 20:12
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.

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 July 25, 2019 11:39
@agnes512 agnes512 force-pushed the model-properties-docs branch from ae22a6e to 9a9ac29 Compare July 25, 2019 13:33
@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 Compare July 25, 2019 16:38
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Nice writeup 👍

docs/site/Model.md Outdated Show resolved Hide resolved
@agnes512 agnes512 force-pushed the model-properties-docs branch from 811015c to 6f84825 Compare July 26, 2019 13:15
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.

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 Compare July 26, 2019 13:38
@agnes512 agnes512 force-pushed the model-properties-docs branch from 4357c38 to 478d44b Compare July 26, 2019 13:42
@agnes512 agnes512 merged commit ee57721 into master Jul 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the model-properties-docs branch July 26, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation of loopback 4 Model is incorrect
5 participants