Skip to content

Conversation

@agnes512
Copy link
Contributor

@agnes512 agnes512 commented Mar 9, 2020

clean up old content and make it less relate to lb3
some content are copied from lb3 and rewrite it with lb4-way

closes loopbackio/loopback.io#907

While doing ^907, I realized that we link to LB3 docs a lot. Considering there are several differences between them and not all users are familiar with LB3, I think it's time to separate them. ( And ofc it is also annoying to jump back and forth between LB3 and LB4 docs)

So I edited the Model Decorator and Property Decorator sections that:

  • copy content form LB3 if we linked it to LB3 before.
  • Remove the unsupported part and rewrite it with Ts-way to LB4.
  • Edit them as a whole to make it more consistent

I tested them briefly before I removing some features/entries. If you happen to know that they are still available but got deleted, please kindly let me know, thanks!!

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 👈

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.

👍 LGTM in general good update! a few nitpicks.
I think lb3 uses scopes instead of scope?

```

### Property Decorator
### Scope
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Scope
### Scopes

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but in LB4 it only works with settings scope:

@model({
  settings: {
    scope: {
      'limit': 2,
    },
    idInjection: true
  }
})

:( that's why I made them scope instead of scopes

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! remembered, LB3 supports both scopes and scope:

  • scopes: a collection of predefined scopes
  • scope: the default scope

So your change makes perfect sense 💯

```ts
{
name: "Product",
scope: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
scope: {
scopes: {

Copy link
Contributor

Choose a reason for hiding this comment

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

And I think you need to give each scope a name?
Like

"scopes": {
	"vips": {"where": {"vip": true}}, 
	"top5": {"limit": 5, "order": "age"}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this part works in LB4, at least from what I tried.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 take my comment back :p

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to double check this part: in LB3 we allow:

"scopes": {
	"vips": {"where": {"vip": true}}, 
	"top5": {"limit": 5, "order": "age"}
}

then the scopes can be used as

User.scope("vips", { where: { vip: true } });
User.scope("top5", { limit: 5, order: "age" });

I don't think this is valid in LB4 because:

  • User model, or any lb4 model, doesn't have such method scope
  • LB4 uses repositories to query data. So User.scope won't work.

Is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Defining

"scopes": {
	"vips": {"where": {"vip": true}}, 
	"top5": {"limit": 5, "order": "age"}
}

is equivalent to

// please note User.scope defines scope, not apply scope
User.scope("vips", { where: { vip: true } });
User.scope("top5", { limit: 5, order: "age" });

After defining them, you can call APIs like User.vips() to query with the predefined filter.

But your conclusion is correct 👌 we don't have such APIs in User repository. Therefore scopes won't be supported. Only specifying the default scope works.

@agnes512
Copy link
Contributor Author

Enhanced the docs for required and default fields that we discussed in #3058 in commit 685361a

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.

💯 LGTM

@agnes512 agnes512 mentioned this pull request Mar 11, 2020
7 tasks
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 work 👏

@agnes512
Copy link
Contributor Author

Even the settings can be imported from LB3, see https://github.com/strongloop/loopback-next/pull/4737/files#r383816292, I've decided to remove idInjection. Because even the DB would have an id property added to the model automatically with idInjection: true set, the id property is still needed in the model definition so that the repository/controller can be set afterwards.

We can revert the change if ^ the conclusion is incorrect.

clean up old content and make it less relate to lb3
some content are copied from lb3 and rewrite it with lb4-way
@agnes512 agnes512 merged commit 6fe1f52 into master Mar 12, 2020
@agnes512 agnes512 deleted the update-models branch March 12, 2020 15:08
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.

Broken anchor link

5 participants