-
Notifications
You must be signed in to change notification settings - Fork 8
Widget routes #33
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
base: master
Are you sure you want to change the base?
Widget routes #33
Conversation
jrobcodes
left a comment
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.
database/schema.js
Outdated
| @@ -0,0 +1,23 @@ | |||
| const mongoose = require('mongoose') | |||
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.
Could we name this file something that is more illustrative about what the file is responsible for? Models? Schema?
As well, I think we need to look at strategies for connecting to the DB once in the application, instead of opening new connections in each module - please connect with @harmanlearns and @jessehall3 to discuss.
database/schema.js
Outdated
|
|
||
| mongoose.connect('mongodb://localhost/lizardboard') | ||
| const db= mongoose.connection | ||
| db.on('error', console.error.bind(console,'connection error:')) |
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.
This is connection configuration stuff, probably belongs in a separate module (see comment above).
database/schema.js
Outdated
| db.once('open', function() { | ||
| console.log('connected') | ||
| }) | ||
|
|
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.
Extra whitespace
database/schema.js
Outdated
| const widgetSchema = mongoose.Schema ({ | ||
| type: String, title: String, size: String, contents: String | ||
| }) | ||
| const userSchema = mongoose.Schema ({ |
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.
Add whitespace. Having said that, two different Schemas should probably be defined in separate modules (along with their models, perhaps?).
database/schema.js
Outdated
| username: String, | ||
| widgets: [ widgetSchema ] | ||
| }) | ||
| const userWidgets = mongoose.model('userWidgets', userSchema) |
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.
Models should be singular UpperCamelCase.
| ) | ||
| }) | ||
|
|
||
| /* UPDATE widgets */ |
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.
Remove comment
| }) | ||
|
|
||
| /* UPDATE widgets */ | ||
| router.post('/:userId/widgets/:widgetId/update', ( request, response, next ) => { |
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.
See note above about RESTful endpoints, removing upsert, etc.
| userWidgets.findOneAndUpdate( | ||
| { _id: userId, widgets: { $elemMatch:{ _id: widgetId }}}, | ||
| { | ||
| $set: { |
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.
Doesn't mongoose provide a much better API for updating than $set? If not, we're going to throw it out...
| }) | ||
|
|
||
|
|
||
| /* DELETE widgets */ |
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.
Remove comment and extraneous whitespace.
|
|
||
|
|
||
| /* DELETE widgets */ | ||
| router.post( '/:userId/widgets/:widgetId/delete', ( request, response, next ) => { |
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.
See notes above re: REST, mongoose API, ES6.
…hem into separate files
No description provided.