-
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
fix(sequelize): extend Inclusion interface with "required" property #10192
fix(sequelize): extend Inclusion interface with "required" property #10192
Conversation
This resolves errors such as the following when trying to use the new property with a repository call: ```ts Type '{ relation: string; scope: { where: { id: number; }; }; required: true; }' is not assignable to type 'InclusionFilter'. Object literal may only specify known properties, and 'required' does not exist in type 'Inclusion'. 53 required: true ``` Signed-off-by: KalleV <[email protected]>
Pull Request Test Coverage Report for Build 6983324259Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
extensions/sequelize/src/types.ts
Outdated
declare module '@loopback/repository' { | ||
interface Inclusion { | ||
/** |
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.
Why does it need to affect the types of core repository module, instead of type extension in the Sequelize repository itself. For the respective methods (find()
, findOne()
, and findById()
).
I'm not certain about the behaviour this code will have but It can reflect to non sequelize repositories as well, which might not be intended here.
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.
@shubhamp-sf Thanks for reviewing this. Yes, the TS declaration merging approach affects non-sequelize repositories too. Extending the method interfaces is much better, however, it doesn't cover the relation query "find" case like:
myRepository(foreignKey).find({
include: [
{
// ...
required: true
}
]
}
I tried a couple of approaches for that case. Perhaps defining new interfaces like "SequelizeHasManyRepositoryFactory" that extend the find
method in the underlying interfaces such as HasManyRepository
?
Signed-off-by: KalleV <[email protected]>
extensions/sequelize/src/sequelize/sequelize.repository.base.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: KalleV <[email protected]>
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
Signed-off-by: KalleV <[email protected]>
@shubhamp-sf, I'm merging this PR based on your approval as you're the code owner. |
@KalleV, thanks for your contribution. Your PR has landed! 🎉 |
Partial fix for: #10193
This resolves Typescript errors such as the following when trying to use the new property with a repository call using "find", "findOne", "findById":
Reference:
https://loopback.io/pages/en/lb4/readmes/loopback-next/extensions/sequelize/
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈