-
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): created hasAndBelongsToMany following convention pa… #2315
feat(repository): created hasAndBelongsToMany following convention pa… #2315
Conversation
hey @codejamninja , I'm currently working on the design and on the repository interface. |
@TomerSalton, Absolutely! Maybe we should sync up over a call sometime too. |
@guitarhero1 Hey, please don't make meaningless posts that contribute nothing to the conversation. If you are attempting to "bump" the topic, please consider simply registering interest using the reaction feature. All you've done here is add a pointless notification to everyone watching the thread(as have I). This code will likely be reviewed as workloads allow, not on demand. Apologies for the extra notification form this post. |
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.
@codejamninja I think it would be easier for all if hasAndBelongsToMany is implemented in smaller incremental steps. This pull request is a good start, I am proposing to make few changes and land it, so that other work can be based on the interfaces implemented here.
My main concern is whether getTargetRepository
+ constraint
is sufficient to build HasAndBelongsToManyRepository
. Is there a way how to leave that decision out of this initial pull request? For example, we can leave DefaultHasAndBelongsToManyRepository
out of this pull request, but I am not sure if there will be enough meaningful code left after that change? Alternatively, maybe we can change the repository class to throw "Not implemented" error for now and clearly say in the class tsdoc that the API is experimental and may change within semver-minor releases.
Thoughts?
/cc @raymondfeng
@@ -210,6 +213,71 @@ export class DefaultCrudRepository<T extends Entity, ID> | |||
); | |||
} | |||
|
|||
/** | |||
* @deprecated |
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.
Let's not add methods that are already deprecated at creation time. Please remove _createHasAndBelongsToManyRepositoryFactoryFor
from the pull request.
* the target repository instance | ||
*/ | ||
constructor( | ||
public getTargetRepository: Getter<TargetRepository>, |
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.
I am not sure if it's enough to provide target repository getter, don't we also need getter for the intermediate ("through") repository?
@codejamninja Thank you so much for the efforts and patience. I'll find some time to review it tomorrow. |
@raymondfeng no worries and thanks. If you wouldn't mind, review the |
We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with
Please refer to this docs page for details. Thanks! |
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
I created some boilerplate for the hasAndBelongsToMany relation using the hasMany relation. Currently this contains the identical functionality as the hasMany relation.
See also #2308
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated