fix(repository): clarify argument type for `@repository` #1213

shimks · 2018-04-02T03:38:13Z

This PR splits @repository decorator into two overloaded methods for clarity in their two separate use cases: repository injection and repository generation based on a model and a datasource.

I had a question come up while I was working on the PR. Is advocating repository injection through @repository(MyRepository.name) the approach we want to take? I'd think that this is something that would go against the principles of Inversion of Control, which is what our Context system is based on.

I thought the point of the context system was to allow different components of an app to be swapped out in a single file. For example, if a user has a test repository and a production repository that he needs to swap out for deployment, our Context system should theoretically provide a single place for the user to swap out the repositories. In the case with @repository(TestRepository.name), the user would have to also go into the controller to physically change TestRepository to ProductionRepository.

Maybe my understanding of Inversion of Control or our selling point of the framework is wrong, but I'd like to hear what others think about this conflict between what our framework is designed for and how we're designing the framework.

connected to Inject repositories registered via app.repository() API #744

Checklist

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 packages/example-* were updated

virkt25 · 2018-04-02T16:53:33Z

packages/repository/src/decorators/repository.ts

    target: Object,
    key?: symbol | string,
-    descriptor?: TypedPropertyDescriptor<Repository<T>> | number,
+    descriptor?: TypedPropertyDescriptor<BoundValue> | number,


Why are we changing this to BoundValue since it's type is any.

Honestly, descriptor argument is pretty irrelevant considering we're never going to be using it as a propertyDescriptor since @repository has no use cases for decorating methods themselves (and if a user tries it, it errors out). My reasoning for replacing Repository<T> was to get rid of the usage of generics since we don't need it at all (at least from what I understand).

I might change the argument name descriptor to something else so that it's less confusing.

virkt25 · 2018-04-02T16:54:39Z

packages/repository/src/decorators/repository.ts

+export type RepositoryDecorator = (
+  target: Object,
+  key?: string | symbol,
+  descriptorOrIndex?: TypedPropertyDescriptor<BoundValue> | number,


Any reason we can't return the type as TypedPropertyDescriptor<Repository<T>>?

^^^ same as reasoning above: to get rid of generics

Because github hid my comments, I'm copypasting from above comment:

Honestly, descriptor argument is pretty irrelevant considering we're never going to be using it as a propertyDescriptor since @repository has no use cases for decorating methods themselves (and if a user tries it, it errors out). My reasoning for replacing Repository<T> was to get rid of the usage of generics since we don't need it at all (at least from what I understand).

I might change the argument name descriptor to something else so that it's less confusing.

Maybe I should put this PR on hold for the author of #744 (@bajtos) to hear his inputs on taking in Class.name as the argument

virkt25 · 2018-04-02T16:57:57Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs


  constructor(
-    @inject('repositories.<%= repositoryName %>')
+    @repository(<%= repositoryName %>.name)


-1

Any reason we can't just do the .name in our implementation of the @repository decorator rather than have the user add it? Poor UX imo and requires people to understand what is happening here.

We should be able to resolve a Dependency given a Class imo. @repository(UserRepository).

Out of scope of this PR but I think this can apply in general to @inject as follows @inject(UserController) should be enough for us to be able to resolve this type. IIRC Angular's DI framework is capable of resolving an instance given a Class.

Although I agree that the decorator should be able to resolve from just the class, it's outside of the scope for tasks in #744. I'll agree to change the @repository arguments in this PR so that it can resolve the dependencies just from the class, but only if everyone unanimously agrees that this is our approach going forward.

It makes sense to use the class for DI. However, I think we should create an issue for it and address it separately to get this patch landed for its intended purpose.

+1 I originally intended to get miroslav's feedback to TV's proposal, but since that might not be coming soon I think I'd like to go ahead with the original plan

+1 for allowing @repository(UserRepository), +1 for leaving it out of scope of #744 and creating a new (follow-up) issue.

virkt25 · 2018-04-02T17:08:09Z

I agree that it seems like it's making DI a little more brittle if we follow this approach BUT there are ways around it still.

A user can bind a TestRepository to the same key by doing app.bind('repositories.UserRepository').to(TestRepository)
OR
A user can pass an instantiated version of TestRepository to the constructor of where @repository is being used.
The test repository can be named the same thing and bound after the original app which would result in the same key with the new mock repository.

IoC is maintained regardless as the dependency is declared in the constructor signature instead of just instantiating a class directly in the constructor yourself which would render all of the above ways useless.

b-admike · 2018-04-02T17:06:46Z

packages/repository/src/decorators/repository.ts

+ *     @repository(Product, mySqlDataSource)
+ *     public prodRepo: DefaultCrudRepository<
+ *       Product,
+ *       typeof Customer.prototype.id


typeof Product.prototype.id

b-admike · 2018-04-02T17:22:54Z

I had a question come up while I was working on the PR. Is advocating repository injection through @repository(MyRepository.name) the approach we want to take? I'd think that this is something that would go against the principles of Inversion of Control, which is what our Context system is based on.

I agree with what @virkt25 said. One idea that comes to mind if you'd like to use a file to swap out test/production repositories is to use a generic key name runtimeRepository for injection and bind the different repositories to it based on perhaps environment variable(s).

b-admike · 2018-04-03T17:51:48Z

docs/site/Decorators.md

-[`@repository(model: string | typeof Entity, dataSource?: string | juggler.DataSource)`](http://apidocs.loopback.io/@loopback%2frepository/#1503)
+
+```ts
+@repository(modelOrRepo: string | typeof Entity, dataSource?: string | juggler.DataSource)


did you mean to remove the link to apidocs?

b-admike · 2018-04-03T17:53:58Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs


  constructor(
-    @inject('repositories.<%= repositoryName %>')
+    @repository(<%= repositoryName %>.name)


It makes sense to use the class for DI. However, I think we should create an issue for it and address it separately to get this patch landed for its intended purpose.

b-admike

I'm fine with this patch, but would definitely like to see @raymondfeng or @bajtos chime in when they're back.

virkt25 · 2018-04-06T03:03:21Z

I would make an argument for not passing in the .name string into the @repository decorator on the basis of #1215 that was merged. getRepository takes in just the Class you are trying to resolve, the decorator should have similar usage / behaviour since it more or less is doing the same thing.

It is poor UX to have one approach require a string and another just the Class itself.

shimks · 2018-04-06T14:58:53Z

Just to make sure that we're on the same page, the implementation of @repository had always taken in a string as an argument. This PR does not add in a new @repository decorator.

I too think that it is poor UX to force users to pass in MyRepository.name instead of the just the constructor itself. I initially wanted to just give benefit of the doubt that there is a reason that we're explicitly passing in the name of the constructor, but I'll add in a commit to make this change. If @bajtos or @raymondfeng doesn't like it, I'll just revert the commit then.

bajtos · 2018-04-09T14:56:02Z

packages/repository/src/decorators/repository.ts

    target: Object,
    key?: symbol | string,
-    descriptor?: TypedPropertyDescriptor<Repository<T>> | number,
+    descriptorOrIndex?: TypedPropertyDescriptor<BoundValue> | number,


Please use any instead of BoundValue. BoundValue represents something that's bound in a Context. Here, you are describing a property of some type, not a value bound via ctx.bind().

virkt25 · 2018-04-09T18:08:46Z

docs/site/Controller-generator.md

  constructor(
-    @inject('repositories.TodoRepository')
+    @repository(TodoRepository)
    public todoRepository: TodoRepository,


Is the type not inferred to be TodoRepository?? -- Is there a way for it to be inferred automatically now?

This applies to all instances below.

That is a very useful feature that I've not thought about. I think this task has been scope creeped enough that I'll create a new issue to implement this feature.

virkt25 · 2018-04-09T19:08:22Z

packages/repository/test/unit/decorator/repository-with-di.ts

 describe('repository class', () => {
  let ctx: Context;

  before(function() {


Can you refactor function to a named function and call that from the before. Something like givenCtx.

See our test guidelines: http://loopback.io/doc/en/contrib/style-guide.html#hooks

shimks · 2018-04-10T15:25:13Z

@raymondfeng Can I get your review on this PR? Thanks

virkt25 · 2018-04-10T17:22:22Z

packages/repository/src/decorators/repository.ts

-        // inject('repository-factory', meta)(target, key!, descriptor);
-        inject('', meta, resolve)(target, key!, descriptor);
-        // throw new Error('@repository(model, dataSource) is not implemented');
+        // inject('repository-factory', meta)(target, key!, descriptorOrIndex);


You can get rid of this line

shimks requested review from bajtos and raymondfeng as code owners April 2, 2018 03:38

shimks added the needs-priority label Apr 2, 2018

0candy added review and removed needs-priority labels Apr 2, 2018

virkt25 reviewed Apr 2, 2018

View reviewed changes

b-admike reviewed Apr 2, 2018

View reviewed changes

b-admike reviewed Apr 3, 2018

View reviewed changes

shimks force-pushed the repository-decorator branch 2 times, most recently from 184a17b to 6fc8de6 Compare April 3, 2018 19:17

b-admike approved these changes Apr 4, 2018

View reviewed changes

bajtos reviewed Apr 9, 2018

View reviewed changes

shimks force-pushed the repository-decorator branch 2 times, most recently from d2f7687 to bc43e0e Compare April 9, 2018 18:03

virkt25 reviewed Apr 9, 2018

View reviewed changes

shimks force-pushed the repository-decorator branch from bc43e0e to 763a6ab Compare April 9, 2018 19:32

shimks force-pushed the repository-decorator branch from 763a6ab to d845eaa Compare April 10, 2018 16:06

virkt25 reviewed Apr 10, 2018

View reviewed changes

virkt25 approved these changes Apr 10, 2018

View reviewed changes

b-admike approved these changes Apr 10, 2018

View reviewed changes

raymondfeng approved these changes Apr 10, 2018

View reviewed changes

feat(repository): have @repository take in constructor as arg

c8f27a9

shimks force-pushed the repository-decorator branch from 86837c1 to c8f27a9 Compare April 10, 2018 20:14

shimks merged commit 3db07eb into master Apr 10, 2018

shimks deleted the repository-decorator branch April 10, 2018 20:41

shimks removed the review label Apr 10, 2018

fix(repository): clarify argument type for @repository #1213

fix(repository): clarify argument type for @repository #1213

Uh oh!

Conversation

shimks commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-admike commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Apr 6, 2018

Uh oh!

shimks commented Apr 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fix(repository): clarify argument type for `@repository` #1213

fix(repository): clarify argument type for `@repository` #1213

shimks commented Apr 2, 2018 •

edited

Loading

shimks Apr 2, 2018 •

edited

Loading

shimks Apr 3, 2018 •

edited

Loading

shimks Apr 2, 2018 •

edited

Loading

virkt25 Apr 9, 2018 •

edited

Loading