Repo class factory improvements

[bajtos]

A proposal how to improve several aspects of #4792.

@raymondfeng if you accept my suggestions, then please preserve my co-authorship in the git commit message (see https://github.blog/2018-01-29-commit-together-with-co-authors/)

[emonddr]

@bajtos @raymondfeng ,
If this is applied, we may need to revisit the LB4 mixin class factory functions in : https://loopback.io/doc/en/lb4/migration-models-mixins.html .

[raymondfeng]

So TypeScript cannot infer the generic types from the parameters in this case?

[raymondfeng]

To make inference happy, I have to do the following:

function castDummyRepository<M extends Model>() {
  return class extends DummyCrudRepository<M> {};
}

const AddressRepository = defineCrudRepositoryClass(
  Address,
  castDummyRepository<Address>(),
);

[raymondfeng]

TypeScript does not allow typeof DummyCrudRepository<M>.

[bajtos]

So TypeScript cannot infer the generic types from the parameters in this case?

Unfortunately not. I think we should report a bug to TypeScript (if it wasn't already reported).

To make inference happy, I have to do the following (...)

IMO, castDummyRepository does not solve the problem of people forgetting to specify the repository type and relying on the compiler to infer it. Once they realize some extra effort is needed, then I think it's best to provide generic type parameters and there is no need to introduce cast*Repository functions.

[raymondfeng]

Same question here.

[bajtos]

If this is applied, we may need to revisit the LB4 mixin class factory functions in : https://loopback.io/doc/en/lb4/migration-models-mixins.html .

@emonddr I am little bit confused, how is this new code related to define*Repository functions related to the documentation on mixin migration? AFAIK, the migration guide is not affected by these changes at all. What am I missing?

[bajtos]

I was thinking about this (and your) pull request last week and I think we have over-engineered this feature. I see two main use cases we need to support:

1. The user wants to define a new repository class extending their custom repository base class.
2. The user wants an easy way how to create a model-bound repository using the built-in CRUD or KeyValue implementation.

If they want to use a custom repository extending DefaultCrudRepository base class, then there is no need to have a specialized defineEntityCrudRepository helper, because they can use the generic helper created to support the first use case in my list above.

With that in mind, I added a new commit 078dfba to greatly simplify the new API.

@raymondfeng PTAL at my proposed version and also at my comments below 👇

[bajtos]

There is no need to have specialized types for KeyValue and CRUD repositories, we can use a generic Repository type as the starting point.

I am not comfortable using RepositoryClass name, because it's too ambiguous and possibly confusing, since it uses different constructor argument than PersistedModelClass and friends.

I was looking for a name that would describe the fact that the repository class is already bound to a given model. ModelRepositoryClass did not feel right either, so I settled on NamedRepositoryClass. Feel free to propose a better name.

[raymondfeng]

If it's NamedRepositoryClass, should we add name: string explicitly?

What about GeneratedRepositoryClass?

[bajtos]

Maybe we should take another angle. My intention is to distinguish between a generic repository class that works for any Model/Entity and a repository class that's already specialized for (bound to) a given Model/Entity.

In that light, perhaps BoundRepositoryClass or SpecializedRepositoryClass would be better names?

[bajtos]

While this works for Entities, it won't work for other subclasses that people may use for their base repository classes.

Consider the following:

class MyAuditedEntityRepository<E extends AuditedEntity, /*...�*/> extends DefaultCrudRepository<E, /*...*/> {
  constructor(modelClass: typeof AuditedEntity, dataSource: juggler.dataSource) {
    // ...
  }
  // ...
}

To support such use case, I think it's best to use M extends typeof Model as the generic type parameter.

[bajtos]

Similarly here, we need M to represent the class constructor, not the class instance.

[raymondfeng]

@bajtos I have rebased this branch against latest master.

[bajtos]

Merged manually.