feat(repository): add getRepository function to mixin #1215
Conversation
shimks
changed the title
shimks
changed the title
| * @param repo The repository class to retrieve the instance of | ||
| */ | ||
| // tslint:disable-next-line:no-any | ||
| async getRepository<Repo extends Repository<any>>( |
There was a problem hiding this comment.
if we have to use any for the type, I'd prefer to be a bit more restrictive and put {}
There was a problem hiding this comment.
We can't do that since the generic used here needs to be restricted to Model. The reasoning behind using any was just to replicate style seen in other code, but I'll see if there's a better reason for it.
BTW, if we were to change any with Model, that would require Model type to be imported every time RepositoryMixin is used.
There was a problem hiding this comment.
Okay then I'm for using Model here and fine with it being imported as RepositoryMixin is used. My reason being VS code intelliSense would be able to figure it out and we already have to import juggler and DataSourceConstructor etc. for RepositoryMixin anyway.
There was a problem hiding this comment.
This does mean that the scope of this PR would get much larger since I'd have to go into the docs to make sure that the imports are there. @strongloop/lb-next-dev thoughts before I make this change?
There was a problem hiding this comment.
Ah on that note, if this is something we're doing in our codebase already (losing type safety because of any), we can create an issue to track it and address it separately.
virkt25
left a comment
virkt25
left a comment
There was a problem hiding this comment.
Can you update the AppWithRepository interface with the getRepository function please.
|
@virkt25 Should I also update |
|
Yes please! :) I think the only other one is datasource. |
| * @param repo The repository class to retrieve the instance of | ||
| */ | ||
| // tslint:disable-next-line:no-any | ||
| async getRepository<Repo extends Repository<any>>( |
There was a problem hiding this comment.
Did you consider using R instead of Repo for the generic variable? I went looking for the Repo type not realizing it was a generic like T.
-- Maybe it's just me, but I expect to see generics as one capital letter. Usually T but that's taken in this case so R might make sense.
There was a problem hiding this comment.
Yeah I'll take a look around in the codebase to see how people named their generics.
virkt25
left a comment
virkt25
left a comment
There was a problem hiding this comment.
LGTM, just one comment about variable name choice. I don't think we have guidelines around it but I haven't see a full name used for a generic variable in our code base.
Fixes Enhance Repository Mixin to provide a repository getter #745
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated