-
Notifications
You must be signed in to change notification settings - Fork 0
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
Dbconnection #32
Dbconnection #32
Conversation
I think the answer is Yes. Looking at server.ts, all routes starting with /api will be routed to ApiRouter. After that ApiRouter will direct each route to their corresponding handler, and presumably pass all the models to each handler as well.
However it's probably a good idea to get the author of this code which is @shangzhel i'm assuming to clarify and confirm on this issue. |
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.
After some test, I've successfully made the collection appear on my Compass by inserting an document into the collection. I was expecting the collection to appear straight away after db.model but I guess this works too.
Everything else looks good to me too.
Indeed it is not perfect in terms of extensibility. The Mongoose models could be exposed through property getters, and the getters themselves located in an interface so that the In import { Model } from 'mongoose';
interface IDbProvider {
get User(): Model<IUser>;
} In import { IDbProvider } from './db-provider';
class ApiRouter implements IDbProvider {
// ...
get User() {
return this.userModel;
}
} I was going to make this change when I set up testing because the routes need to be testable without actually making a database connection in test cases. |
I could make these changes and make a new commit if you'd like. |
@shangzhel Actually, considering that you'll implement that feature in CI. I will refrain from making these changes else we run into conflicts. |
Let's leave this PR open until after the CI PR gets merged, then we can write unit tests before merging. |
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.
Needs a few fixes.
server/src/models/card.ts
Outdated
company: string; | ||
fields: ICardField[]; | ||
tags: ObjectId[]; | ||
image?: Buffer; |
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.
Move this property up to before the arrays.
server/src/models/card.ts
Outdated
name: { type: String, require: true }, | ||
phone: { type: String, require: true }, | ||
email: { type: String, require: true }, | ||
image: Buffer, |
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.
Move this element down to before the fields
value.
server/src/models/tag.ts
Outdated
} | ||
|
||
export const tagSchema = new Schema<ITag>({ | ||
tagName: String, |
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.
Rename property to match ITag interface definition.
server/src/models/tag.ts
Outdated
|
||
export const tagSchema = new Schema<ITag>({ | ||
tagName: String, | ||
colour: String, |
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.
Same as above, rename property to match interface definition.
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.
Just one change, otherwise it looks good.
server/src/models/card.ts
Outdated
company: { type: String, require: true }, | ||
image: Buffer, | ||
fields: { | ||
type: [new Schema<ICardField>({ |
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.
Move the initialization of the ICardField schema into another constant above like const cardFieldSchema = ...
. It does not need to be exported.
Don't know how your code got past the linter pre-commit hook, but I can fix it for you. |
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.
Looks good to me.
Personal observations
I would like to bring attention to
api-router.ts
where the database models are meant to be generated.Models generated within this file are bound to individual api Handlers through the bind method. Hence for each ApiRequestHandler type, the model can be accessed through
this.<model property in api-router.ts>
.Is this an intended outcome? For each new model required it would have to update this class. So far we have a User model and nothing else. But in terms of extending the DB model for further collections, how do you think this will interact?