Skip to content

Conversation

@jannyHou
Copy link
Contributor

connect to #4521, prepare for User model migration

address comment #4706 (comment)

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 👈

@jannyHou jannyHou force-pushed the refactor/migrate-model-acl branch from 697cbeb to 0a817d0 Compare March 11, 2020 15:46
@jannyHou jannyHou self-assigned this Mar 11, 2020
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.

It's nice to see an example uses CLIs to import models from LB3 and also create LB4 relations 👍 I like the improvement of consistency.

Comment on lines 9 to 15
@model({settings: {strict: false, idInjection: true}})
export class Project extends Entity {
@property({
type: 'number',
id: true,
id: 1,
generated: false,
updateOnly: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few concerns here but they might not be related to the PR itself:

  • Is settings. idInjection from model migration? While I was doing task docs: update Models.md #4829, I removed idInjection from the LB4 docs because it doesn't work when I tested it ( I might test it incorrectly)
  • LB3 uses id: 1 cause it allows composite ids, and 1 indicates true and also its index. Ideally we should just use id: true for imported models.

Copy link
Contributor Author

@jannyHou jannyHou Mar 11, 2020

Choose a reason for hiding this comment

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

@agnes512 I didn't have it before, it's generated by the migration CLI. If it's not correct, I think we'd better fix it from CLI side.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, but I don't know how much effort it needs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah got it after reading the story #4829 , let me remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I check what @bajtos did in his PR: https://github.com/strongloop/loopback-next/pull/4737/files#r383816292

And this is what I tried:

  1. Define a LB4 model with idInjection: true
  settings: {
    idInjection: true,
  },
})
export class User extends Entity {
  // @property({   // no id property defined
  //   type: 'number',
  //   id: true,
  //   generated: true,
  // })
  // id?: number;

  @property({
    type: 'string',
  })
  name: string;
...
}
  1. then run migration-> failed.

I am not hundred percent sure if idInjection works in LB4. I am okay to keep it for sake of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agnes512 many thanks for double checking and verifying with code!

I am okay to keep it for sake of consistency.

If it causes failure I'd like to remove in the example, I can investigate a bit (after having PR ready for #4521) and create issue/PR to fix if needed.

@jannyHou jannyHou force-pushed the refactor/migrate-model-acl branch from edf5feb to 103d765 Compare March 11, 2020 21:15
```
- For all the three models above, allow generating their `id` field:
```ts
@property({
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: there's an extra spacing.

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.

LGTM. Thanks!

@jannyHou jannyHou force-pushed the refactor/migrate-model-acl branch from 103d765 to 5b7321d Compare March 12, 2020 17:46
@jannyHou jannyHou merged commit 7c11904 into master Mar 12, 2020
@jannyHou jannyHou deleted the refactor/migrate-model-acl branch March 12, 2020 18:54
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.

4 participants