Conversation
697cbeb to
0a817d0
Compare
agnes512
left a comment
There was a problem hiding this comment.
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.
| @model({settings: {strict: false, idInjection: true}}) | ||
| export class Project extends Entity { | ||
| @property({ | ||
| type: 'number', | ||
| id: true, | ||
| id: 1, | ||
| generated: false, | ||
| updateOnly: true, |
There was a problem hiding this comment.
I have a few concerns here but they might not be related to the PR itself:
- Is
settings. idInjectionfrom model migration? While I was doing task docs: update Models.md #4829, I removedidInjectionfrom the LB4 docs because it doesn't work when I tested it ( I might test it incorrectly) - LB3 uses
id: 1cause it allows composite ids, and 1 indicates true and also its index. Ideally we should just useid: truefor imported models.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
True, but I don't know how much effort it needs
There was a problem hiding this comment.
ah got it after reading the story #4829 , let me remove them.
There was a problem hiding this comment.
I check what @bajtos did in his PR: https://github.com/strongloop/loopback-next/pull/4737/files#r383816292
And this is what I tried:
- 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;
...
}
- 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.
There was a problem hiding this comment.
examples/access-control-migration/src/repositories/project.repository.ts
Show resolved
Hide resolved
edf5feb to
103d765
Compare
docs/site/migration/auth/example.md
Outdated
| ``` | ||
| - For all the three models above, allow generating their `id` field: | ||
| ```ts | ||
| @property({ |
There was a problem hiding this comment.
nitpick: there's an extra spacing.
103d765 to
5b7321d
Compare
connect to #4521, prepare for User model migration
address comment #4706 (comment)
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machineNew tests added or existing tests modified to cover all changesAPI Documentation in code was updatedAffected artifact templates inpackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈