-
Notifications
You must be signed in to change notification settings - Fork 0
Implement caching for Question page #18
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?
Conversation
app/views/comments/_list.slim
Outdated
| hr style="margin:2px 4px 0 4px" | ||
| h4 Comments: | ||
| div id="#{commentable.entity}-comments-#{commentable.id}" style="margin-left:40px" | ||
| - commentable.comments.each do |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.
А почему бы весь список не закэшировать?
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.
Да, пожалуй можно поднять повыше, не задевая логики условных отображений и подключения JST. Хотя, оно все равно попадало бы в вышестоящий по вложенности кеш.
| - commentable.comments.each do |comment| | ||
| = render 'comments/comment', comment: comment | ||
| - cache comment do | ||
| = render 'comments/comment', comment: 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.
Лучше кэш переместить внутрь самого паршла, чтобы можно было и в других местах его же использовать.
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.
Пожалуй, да, и першаиал не имеет логики, и в свете предыдущей правки это логично.
app/views/questions/show.slim
Outdated
| = @question.body | ||
| #question-attachments | ||
| = render 'attachments', attachable: @question | ||
| - cache @question do |
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.
Да, соглашусь, лихо скопировал из скринкаста и как раз здесь о логике вьюхи не задумался. Сходу есть вариант не кешировать всю страницу, но он мне не нравится. И подсознательно чувствую, что есть способ сделать ключ кеша, но пока не могу точно сообразить, что в него включить. Постараюсь завтра додумать этот момент, это единственное, что осталось сейчас тут исправить, по другим пунктам уже прошелся.
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.
Размышлял, что лучше заключить в кеши, в итоге сделал примерно как делал всегда -- записал туда (в случае текущего проекта) учствующие в формировании Policy и локальные предикаты. Писал сразу их значения, так как нет перекрывающихся по их количеству блоков. Путь кеша получился полу-понятным, но понятнее все равно не сильно сделаешь, да и не за чем в общем-то. Заодно, обнаружил, что первоначально все сделал достаточно "на скорую руку", не особо задумываясь, поэтому доделал еще связанные модели и выделил кеширующие блоки. Не уверен, что оптимально и они существуют не дублируя друг друга, но зато надежно. Попутно пришлось еще несколько спек поправить.
| = f.text_area :body, class: 'form-control' | ||
| = f.submit 'Save', class: "btn btn-info btn-sm pull-right" | ||
| hr | ||
| - cache [answer, policy(answer).edit?, policy(answer).destroy?, policy(answer).star?, comet] |
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.
Идея хорошая, но в данном случае разве нельзя ограничится чем-то одним из edit? и destroy?.
Хотя, если они ровны, на количество ключей не повлияет все равно.
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.
Да можно в принципе, ключи типа "Answer и 3 boolean-а" нигде не используются больше, но это скорее с целью предусмотреть "на будущее", если вдруг они станут не равны -- формально мы же их так и проверяем разными в паршиале. Тем более, что длина ключа особо не влияет ни на что, так что решил формально учесть, как было, например, с политикой на подписку.
caf6ee3 to
1299e67
Compare
No description provided.