Skip to content

cant get authors to work still#25

Open
scl9087 wants to merge 4 commits intopce-uw-jscript400:masterfrom
scl9087:master
Open

cant get authors to work still#25
scl9087 wants to merge 4 commits intopce-uw-jscript400:masterfrom
scl9087:master

Conversation

@scl9087
Copy link

@scl9087 scl9087 commented Jul 20, 2019

I apologize for this being late. I was sick over the weekend and last week. I am trying to catch up.

const router = require('express').Router({mergeParams: true})
const Books = require('../models/books')


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of how you defined your routes in the books.js route file most of the code in this file will never be called. See my other comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O jeez! That saved me, I felt like I was going crazy! Thank you for your help!

})

router.put('/:id', (req, res, next) => {
router.get('/:bookId/authors/:id', async (req, res, next) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By defining this route in books.js this 'get' will be executed while the 'get' in books.authors.js line 24 will never be called because this is matched first. It's ok to set up routes this way but be aware that this will happen.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now fixed.

})

router.delete('/:id', (req, res, next) => {
router.put('/:bookId/authors/:id', async (req, res, next) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea here, authors/:id will execute here instead of books.authors.js

res.json({ status, author })
})

router.put('/:id', async (req, res, next) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks good but is never running because of the issue posted below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants