Skip to content
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

Livereload sleep #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jamesluberda
Copy link

This change implements the functionality for the --livereload-sleep option proposed in Jekyll PR #7064. More details can be found there. Relevant to this part of the change:

With the option --livereload-sleep [SECONDS] enabled via jekyll serve, jekyll-watch will pass along the specified livereload-sleep value, or a default of 1, to Listen as the wait_for_delay parameter (overriding Listen's default of 0.10 seconds), delaying the callbacks, thus causing auto-regeneration to occur only once across rapid successive changes to a file within the specified "sleep" time. As a result, the client's request is unlikely to occur with an auto-regeneration underway, and will instead receive the updated content as expected, rather than a 404 error due to auto-regenerations clobbering each other.

@jamesluberda
Copy link
Author

Checking in--can we merge this?

@ashmaroli
Copy link
Member

Is this feature dependent on jekyll/jekyll#7064 ?
Or would this work as a standalone feature?

@jamesluberda
Copy link
Author

It is dependent on jekyll/jekyll#7064 to be readily useful, specifically to provide the user the optional sleep parameter via a jekyll serve. That said, in theory, it could be integrated into watch independently without adverse impact, and possibly (I've not explored it), used after a jekyll serve followed by a script that kills any existing watch and restarts it with this parameter included, but I think that would be ugly even if it worked. The design was to have an additional user control over livereload in addition to the existing livereload parameters.

@ashmaroli
Copy link
Member

The reason I asked was because of the following complications:

  • The existing livereload logic is entirely contained within Jekyll core. This plugin wouldn't care about what Jekyll version is being used.
  • Introducing this change would mean introducing a coupling with the jekyll version used. The linked upstream PR is never going to land in Jekyll 3.x (that branch is now in security-maintenance phase) but only in 4.x (if and when accepted upstream).
    So, technically, we can't merge this PR until a decision is made regarding the PR upstream.
  • Lastly, testing this feature and ensuring that changes made upstream do not break this feature becomes harder as there isn't a well-defined API.

I may be overthinking about this but I'm open to accepting a change that doesn't add much complexity and maintenance overhead.

/cc @jekyll/plugin-core

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