feat: move functions to define repository classes from @loopback/rest-crud to @loopback/repository #4792

raymondfeng · 2020-03-04T18:40:38Z

The PR is inspired by:

Spike: Discover and define models at runtime #4235 (comment)
We already have defineModelClass in @loopback/repository

~~For @loopback/rest-crud:~~

BREAKING CHANGE: defineCrudRepositoryClass is now removed from
@loopback/rest-crud. Please use defineEntityCrudRepositoryClass from
@loopback/repository instead.

See some context at #4296

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 👈

nabdelgadir

The code changes look reasonable to me, but I'm confused about the context of the move (I read #4296). Can you explain it a bit more?

packages/repository/src/repository-builder.ts

packages/repository/src/__tests__/unit/repositories/define-crud-repository-class.unit.ts

bajtos · 2020-03-05T08:31:05Z

BREAKING CHANGE: defineCrudRepositoryClass is now removed from
@loopback/rest-crud. Please use defineEntityCrudRepositoryClass from
@loopback/repository instead.

Have you considered preserving backwards compatibility by adding re-export of defineCrudRepositoryClassto @loopback/rest-crud?

I'm confused about the context of the move (I read #4296). Can you explain it a bit more?

+1, can you please @raymondfeng shed more light on your motivations?

Personally, I did consider to move defineEntityCrudRepositoryClass from @loopback/rest-crud to @loopback/repository in the past too. However, I think we will need to make this function more generic in the (near?) future and allow the caller to provide their own repository class to inherit from. I am not sure if such generic function belongs to the repository package?

Let's discuss.

bajtos

@raymondfeng thank you for starting this work, I agree it's a good idea to make our repository-class-factories more flexible.

The most important problem I see in this pull request is that since all changes are bundled in a single commit with BREAKING CHANGE trailer, it will trigger semver-major release of @loopback/repository and include description of breaking changes in repository's release notes. IMO, that's not desired.

I also find the current proposal as changing too many things at once, in a way that does not make it clear where one change ends and another starts. I see the following changes here:

Introduction of new factories (defineCrudRepositoryClass, defineKeyValueRepositoryClass)
Reworking the API to be more extensible and support custom repository base classes to some extent
Reorganization of code, moving the factory function from @loopback/rest-crud to @loopback/repository.

Let's find a way how to organize the patch in such way that each change is isolated and easier to understand.

I am proposing the following plan:

Introduce generic repository-class-factory functions in @loopback/repository (defineCrudRepositoryClass, defineKeyValueRepositoryClass). Make sure they support custom repository base classes both at compile time and at run time. I think this would make for a nice standalone pull request.
Rework @loopback/rest-crud to preserve the API but implement the factory function using the new @loopback/repository APIs. Perhaps deprecate the exported function? If you feel strongly about removing the factory API from @loopback/rest-crud completely, then I am fine with that too. However, I don't want to couple the upcoming semver-major release with landing of this pull request, because it would create unwanted pressure on getting this landed ASAP, instead of taking enough time to iterate until we find a good API & implementation.

packages/repository/src/repository-builder.ts

packages/rest-crud/src/repository-builder.ts

raymondfeng · 2020-03-06T18:35:07Z

@bajtos @nabdelgadir PTAL

bajtos

Good progress 👏

I see several issues with the TypeScript typings used in the new version. The names used also make it easy for the reader to get confused about what interface/type-alias refers to what. Let's improve these aspects please and make sure we also have tests to verify that the compiler will allow users to call the repository classes in the way we would like them to.

packages/repository/src/__tests__/unit/repositories/define-repository-class.unit.ts

packages/repository/src/define-repository-class.ts

packages/rest-crud/README.md

packages/repository/src/define-repository-class.ts

raymondfeng · 2020-03-09T17:44:39Z

@bajtos I believe the latest PR has addressed your comments. PTAL.

Most important problems were addressed.

bajtos

Almost there! I have few more minor comments to consider. PTAL below.

packages/repository/src/__tests__/unit/repositories/define-repository-class.unit.ts

packages/repository/src/define-repository-class.ts

packages/rest-crud/README.md

raymondfeng · 2020-03-17T17:04:13Z

@bajtos PTAL.

bajtos

@raymondfeng I am afraid you still did not addressed my comments asking for support of custom repository methods.

PTAL at #4949, it has three commits with suggestions how to improve your PR.

I think the first can be applied as-is.
I am not sure if the second commit is an improvement or not. When applied, it helped me to catch the problem with AddressRepository using any as the model type, but further investigation revealed that it was a kind of of a coincidence. OTOH, it can create confusion because it changes some repository-related types to accept typeof Model instead of Model as the first generic argument.
The third commit adds tests to verify support for custom repository methods and also fixes the implementation as needed. If you decide to leave the second commit out, then it will need few changes to accommodate that.

packages/repository/src/__tests__/unit/repositories/define-repository-class.unit.ts

emonddr · 2020-03-23T14:09:22Z

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

bajtos · 2020-04-07T16:39:49Z

I rebased the PR on top of the latest master and merged in changes from #4949. The rebase was a bit tricky, I ended up re-creating both commits from scratch.

bajtos

@raymondfeng PTAL, I merged the changes proposed in #4949.

I find the new version pretty rad, I think the only remaining point to discuss is a better name for NamedRepositoryClass.

Thoughts?

bajtos · 2020-04-07T16:41:04Z

packages/repository/src/define-repository-class.ts

+ * @typeParam M - Model class
+ * @typeParam R - Repository class/interface
+ */
+export interface NamedRepositoryClass<


The name of this class is a discussion point we haven't resolved yet. See #4949 (comment).

Cross-posting my latest comment:

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?

Maybe TypedRepositoryClass?

I renamed it to ModelRepositoryClass. Let's just land it - no need to be perfect.

bajtos · 2020-04-07T16:43:42Z

packages/rest-crud/README.md

-respositories using code.
+(`defineCrudRestController` from `@loopback/rest-crud` and
+`defineCrudRepositoryClass` from `@loopback/repository`). These functions will
+help you create controllers and repositories using code.


Should we revert this change since defineCrudRepositoryClass is still exported by @loopback/rest-crud? (Same comment applies to other similar doc changes below.)

I'm fine either way.

Introduce `defineRepositoryClass`, a generic variant of `defineCrudRepositoryClass` that can be used for any base repository implementation. Implement two convenience wrappers for creating default CRUD/KeyValue repository classes: - `defineCrudRepositoryClass` - `defineKeyValueRepositoryClass` Co-authored-by: Miroslav Bajtoš <[email protected]>

…repository` Drop our own implementation in favor of the new generic version. Co-authored-by: Miroslav Bajtoš <[email protected]>

raymondfeng requested review from agnes512, hacksparrow and nabdelgadir as code owners March 4, 2020 18:40

raymondfeng added the breaking-change label Mar 4, 2020

emonddr approved these changes Mar 4, 2020

View reviewed changes

nabdelgadir reviewed Mar 5, 2020

View reviewed changes

packages/repository/src/repository-builder.ts Outdated Show resolved Hide resolved

packages/repository/src/repository-builder.ts Outdated Show resolved Hide resolved

packages/repository/src/__tests__/unit/repositories/define-crud-repository-class.unit.ts Outdated Show resolved Hide resolved

bajtos suggested changes Mar 5, 2020

View reviewed changes

packages/repository/src/repository-builder.ts Outdated Show resolved Hide resolved

packages/rest-crud/src/repository-builder.ts Show resolved Hide resolved

raymondfeng force-pushed the move-repo-class-factories branch 3 times, most recently from d1c58e7 to 5ddf752 Compare March 5, 2020 18:22

raymondfeng requested review from bajtos, nabdelgadir and emonddr March 5, 2020 18:23

nabdelgadir approved these changes Mar 7, 2020

View reviewed changes

emonddr approved these changes Mar 9, 2020

View reviewed changes

bajtos previously requested changes Mar 9, 2020

View reviewed changes

raymondfeng force-pushed the move-repo-class-factories branch from 5ddf752 to c016550 Compare March 9, 2020 17:38

raymondfeng requested a review from bajtos March 9, 2020 20:52

raymondfeng removed the breaking-change label Mar 10, 2020

bajtos reviewed Mar 12, 2020

View reviewed changes

raymondfeng force-pushed the move-repo-class-factories branch 3 times, most recently from 38443b5 to ab4950a Compare March 13, 2020 20:32

raymondfeng mentioned this pull request Mar 14, 2020

How to build models, repositories and controllers dynamically at runtime #4296

Closed

7 tasks

raymondfeng requested a review from bajtos March 16, 2020 22:08

bajtos mentioned this pull request Mar 23, 2020

Repo class factory improvements #4949

Closed

bajtos suggested changes Mar 23, 2020

View reviewed changes

packages/repository/src/__tests__/unit/repositories/define-repository-class.unit.ts Outdated Show resolved Hide resolved

packages/repository/src/__tests__/unit/repositories/define-repository-class.unit.ts Outdated Show resolved Hide resolved

raymondfeng force-pushed the move-repo-class-factories branch from ab4950a to 3ec19d0 Compare April 6, 2020 02:20

bajtos added feature Repository Issues related to @loopback/repository package labels Apr 7, 2020

bajtos force-pushed the move-repo-class-factories branch from 3ec19d0 to 401a81c Compare April 7, 2020 16:39

bajtos reviewed Apr 7, 2020

View reviewed changes

bajtos requested review from bajtos, nabdelgadir and emonddr April 7, 2020 16:49

bajtos assigned raymondfeng and bajtos Apr 7, 2020

raymondfeng force-pushed the move-repo-class-factories branch 2 times, most recently from 9350c5d to 88cce18 Compare April 22, 2020 17:36

raymondfeng and others added 2 commits May 2, 2020 22:10

refactor(rest-crud): use defineCrudRepositoryClass from `@loopback/…

aaf77df

…repository` Drop our own implementation in favor of the new generic version. Co-authored-by: Miroslav Bajtoš <[email protected]>

raymondfeng force-pushed the move-repo-class-factories branch from 88cce18 to aaf77df Compare May 3, 2020 05:11

This comment has been minimized.

Sign in to view

raymondfeng merged commit 9838790 into master May 3, 2020

raymondfeng deleted the move-repo-class-factories branch May 3, 2020 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move functions to define repository classes from @loopback/rest-crud to @loopback/repository #4792

feat: move functions to define repository classes from @loopback/rest-crud to @loopback/repository #4792

raymondfeng commented Mar 4, 2020 •

edited by bajtos

Loading

nabdelgadir left a comment

bajtos commented Mar 5, 2020

bajtos left a comment

raymondfeng commented Mar 6, 2020

bajtos left a comment

raymondfeng commented Mar 9, 2020

bajtos left a comment

raymondfeng commented Mar 17, 2020

bajtos left a comment

emonddr commented Mar 23, 2020

bajtos commented Apr 7, 2020 •

edited

Loading

bajtos left a comment

bajtos Apr 7, 2020

raymondfeng Apr 10, 2020

raymondfeng May 3, 2020

bajtos Apr 7, 2020

raymondfeng Apr 10, 2020

This comment has been minimized.

This comment has been minimized.

feat: move functions to define repository classes from @loopback/rest-crud to @loopback/repository #4792

feat: move functions to define repository classes from @loopback/rest-crud to @loopback/repository #4792

Conversation

raymondfeng commented Mar 4, 2020 • edited by bajtos Loading

Checklist

nabdelgadir left a comment

Choose a reason for hiding this comment

bajtos commented Mar 5, 2020

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Mar 6, 2020

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Mar 9, 2020

bajtos left a comment

Choose a reason for hiding this comment

raymondfeng commented Mar 17, 2020

bajtos left a comment

Choose a reason for hiding this comment

emonddr commented Mar 23, 2020

bajtos commented Apr 7, 2020 • edited Loading

bajtos left a comment

Choose a reason for hiding this comment

bajtos Apr 7, 2020

Choose a reason for hiding this comment

raymondfeng Apr 10, 2020

Choose a reason for hiding this comment

raymondfeng May 3, 2020

Choose a reason for hiding this comment

bajtos Apr 7, 2020

Choose a reason for hiding this comment

raymondfeng Apr 10, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

raymondfeng commented Mar 4, 2020 •

edited by bajtos

Loading

bajtos commented Apr 7, 2020 •

edited

Loading