-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(repository): add interface for hasManyThrough repository #4312
feat(repository): add interface for hasManyThrough repository #4312
Conversation
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
6a2fce6
to
5dec57c
Compare
5dec57c
to
0f3ea26
Compare
Hi. Are there any changes you would like to see to the interface? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @KalleV for the contribution, I'd like to discuss the proposed Repository API design - see my comments below.
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
42224cb
to
1495269
Compare
1495269
to
ea77f8b
Compare
@bajtos Thank you for the review. I addressed the comments. Let me know if you would like any further changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @KalleV for the update.
I would like to simplify the first version of this repository interface and make it more consistent with other relation repositories. This may be controversial, so let's wait for feedback from @raymondfeng @jannyHou @agnes512 and @nabdelgadir before proceeding further.
@strongloop/loopback-maintainers @strongloop/loopback-next may want to chime in too.
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
b64af60
to
95d1dd1
Compare
@bajtos Thank you for the 2nd review. I simplified the interface as suggested and added the throughData property to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, looks good to me.
I really like the idea of providing throughData
in link
. But i think there is also the usecase of updating this throughData
. How to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@derdeka Perhaps the patch method could also pass |
packages/repository/src/relations/has-many/has-many-through.repository.ts
Outdated
Show resolved
Hide resolved
192f60e
to
d41be0c
Compare
We need the corresponding tests. |
How do you propose to test this new interface? AFAIK, we don't have any tests for other relation-repository interface either. From the perspective of testing, it would be better to land this interface together with an implementation, so that we have some coverage. On the other hand, since the new interface isn't used anywhere yet, I think it's ok to tweak or change it if we discover any problems in future pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Please add "EXPERIMENTAL" warning to the API docs, just to be sure.
packages/repository/src/relations/has-many/has-many-through.repository.ts
Show resolved
Hide resolved
The hasManyThrough repository interface defines the actions that can be performed on a many-to-many model relation. This PR is a continuation of loopbackio#2359 and implements a step from loopbackio#2359 (comment).
d41be0c
to
9336c1e
Compare
The hasManyThrough repository interface defines the actions that can be performed on a many-to-many model relation.
This PR is a continuation of #2359 and implements a step from #2359 (comment).
Relates to: #4247
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈