Trigger livereload in sites without pages#8337
Conversation
DirtyF
left a comment
There was a problem hiding this comment.
Don't we need a test for this?
We do. But I couldn't come up with a test implementation.. |
|
@parkr I'm not able to come up a good test for the proposed change here. Will you be able to help..? |
|
@mattr- Requesting your help to add a test for this.. Whatsay? |
|
@parkr Requesting your help to add some test for this. |
lib/jekyll/commands/serve.rb
Outdated
| regenerator = Jekyll::Regenerator.new(site) | ||
| @changed_pages = site.pages.select do |p| | ||
| regenerator.regenerate?(p) | ||
| @changed_pages ||= [] |
There was a problem hiding this comment.
The previous implementation always reset this variable, the new implementation only adds, never removes. Is that intended?
There was a problem hiding this comment.
Good catch! I did not realize that the array never gets reset !!
Definitely not intended. 🤦♂️
|
I'd use mocks to test for this. I'd have a fake regenerator that tells me to regenerate a non-Page object and ensure it's included in the changed pages list? |
|
If this didn't break tests, I'm fine shipping it without an additional one. We can come back to making tests for LiveReload if you don't have time to tackle this. |
|
@jekyllbot: merge +fix |
Ashwin Maroli: Trigger livereload in sites without pages (#8337) Merge pull request 8337
Summary
Livereload should be triggered even if the site doesn't have a single standalone page.
Context
Resolves #8332
/cc @jaybe-jekyll