-
Notifications
You must be signed in to change notification settings - Fork 142
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(query-typegoose): Adds the ability to use discriminators #1321
Conversation
@doug-martin - I see I actually can merge this PR myself, but I'd like your review please, as I'm still not very good with TypeScript and I'm basically cheating by looking at another library for the solution (sort of). If you'd like more detail on that, let me know. I'm also uncertain about having all the tests copied and ran for the discriminated entity. I just thought it would be clear the discriminated entity would work 100% across the board with the query service. If I actually should add more tests, like in e2e, do let me know. Ah, and basically, the goal is to allow for the below discriminator definition: moduleRef = await Test.createTestingModule({
imports: [
TypegooseModule.forRoot(await getConnectionUri(), {
useFindAndModify: false,
useNewUrlParser: true,
useUnifiedTopology: true,
}),
NestjsQueryTypegooseModule.forFeature([
{
typegooseClass: TestEntity,
discriminators: [TestDiscriminatedEntity], // <- this !!!!
},
TestReference,
]),
],
providers: [TestDiscriminatedEntityService, TestReferenceService],
}).compile();
});
This is the actual docs on the topic:
Ah, and lastly, theoretically this could be good information for the docs. If you'd like, I can add a section about it, if you feel it is necessary. Scott |
Pull Request Test Coverage Report for Build 1193990354
💛 - Coveralls |
@JohnMcInall - would you be willing and have the time to look over this PR? Scott |
@smolinari I'm not super familiar with mongoose/typegoose and the more advanced features, but I'll take a look. |
@doug-martin - Ok. I'd appreciate any more experienced insights, even if it's not on the overall functionality, but more on the basic code level. If you say it's ok. I'll get it merged. Scott |
@smolinari I shall take a look when I have a little time today. |
Awesome @JohnMcInall. I appreciate it. Scott |
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.
@JohnMcInall - Puh. Only had a "Resolve Discussion" button and I clicked it and our discussion disappeared. But, I got what you said. Thanks!!! Commit is coming. Just have to figure out the GPG stuff. Been a while since I've worked with Github and PRs.
Scott
@doug-martin - I've gone ahead and merged this. Hope that was ok. Scott |
Fixes #1320
Scott