-
Notifications
You must be signed in to change notification settings - Fork 1
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
Учебный проект: долгая прогулка #19
Учебный проект: долгая прогулка #19
Conversation
…-3 into module6-task2
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.
Так слишком много важных изменений за один раз. Обработка ошибок совсем не ясна. Некоторые вещи не очевидны. Илья надо созвонится.
src/express/routes/main-routes.js
Outdated
@@ -9,9 +9,12 @@ const ROOT = `main`; | |||
const mainRouter = new Router(); | |||
|
|||
mainRouter.get(`/`, async (req, res) => { | |||
const _offers = await api.getOffers(); | |||
const [_offers, categories] = await Promise.all([ |
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.
У нас это не класс, почему подчеркивание? Это же не приватный метод
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.
Потому что шаблонизатор «Паг 3» выдаёт ошибку, если переменная начинается с ключевого слова: в моём случае с of
.
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.
так тогда передавай {_offers: offers} в сам шаблон. а лучше (offersForPug: offers) чтобы понять было, что не просто так это сделали.
src/express/routes/my-routes.js
Outdated
@@ -15,7 +15,7 @@ myRouter.get(`/`, async (req, res) => { | |||
}); | |||
|
|||
myRouter.get(`/comments`, async (req, res) => { | |||
const _offers = await api.getOffers(); | |||
const _offers = await api.getOffers({comments: true}); | |||
|
|||
res.render(`${ROOT}/comments`, {_offers: _offers.slice(0, 3)}); |
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.
Нужен поясняющий комментарий, почему мы так делаем. Отрезаем только первые три части массива.
@@ -25,7 +25,12 @@ const storage = multer.diskStorage({ | |||
|
|||
const upload = multer({storage}); | |||
|
|||
offersRouter.get(`/add`, (req, res) => res.render(`${ROOT}/add`)); | |||
offersRouter.get(`/add`, async (req, res) => { | |||
const categories = await api.getCategories(); |
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.
Вопрос, что будет если сервис вернет ошибку?
Каждый await это потенциальная ошибка и обработки ее или нет или она не очевидна.
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.
Связался с автором курса — будем обрабатывать ошибки и делать валидацию в следующем модуле.
Остальное исправил.
src/express/routes/offers-routes.js
Outdated
offersRouter.get(`/:id`, async (req, res) => { | ||
const {id} = req.params; | ||
|
||
const _offer = await api.getOffer(id, true); |
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.
Опять все вместе подчеркивание и await
src/express/routes/offers-routes.js
Outdated
}); | ||
|
||
offersRouter.get(`/category/:id`, (req, res) => | ||
res.render(`${ROOT}/category`)); |
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.
Мы в шаблон ничего не передаем? Вроде же конкретную категорию просим :id
src/service/api/offers/mockData.js
Outdated
`Книги` | ||
]; | ||
|
||
const offer1Title = `Куплю антиквариат`; |
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.
Опять число. это же мок? offerFirstMockTitle, что-то типа такого. прочитал переменную и понял для чего она.
route.get(`/`, (req, res) => { | ||
const offers = service.findAll(); | ||
route.get(`/`, async (req, res) => { | ||
const {comments} = req.query; |
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.
вопрос. Где обработка если comments нет?, что будет если сервис выдал ошибку?
route.post(`/`, offerValidator, (req, res) => { | ||
const offer = service.create(req.body); | ||
route.post(`/`, offerValidator, async (req, res) => { | ||
const offer = await service.create(req.body); |
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.
А если не создал?
src/service/data-service/comments.js
Outdated
offer.comments = offer.comments.filter((item) => item.id !== commentId); | ||
|
||
return dropComment; | ||
return wasDropped; |
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.
Зачем это? Почему бы просто не передать
return Boolean(deletedRows);
constructor(offers) { | ||
this._offers = offers; | ||
constructor(sequelize) { | ||
this._Offer = sequelize.models.Offer; |
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.
какая же сильная связность.
🎓 Учебный проект: долгая прогулка