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

Allow delaying reprocessing on file change. #564

Closed
wants to merge 0 commits into from
Closed

Conversation

veleek
Copy link
Contributor

@veleek veleek commented Jun 9, 2019

Resolves #559

Add support for specifying a delay after a file change occurs after which Eleventy will start reprocessing the input files. As a super simple example, if you have two files open in an editor and you are running eleventy --serve and then you execute a save all eleventy will:

  • Receive a change notification for the first file.
  • Start reprocessing all files.
  • Receive a change notification for the second file.
  • Queue up a reprocess
  • Finish processing all files INCLUDING the already modified second file.
  • "Notice" that a file was saved while eleventy was running and then start reprocessing.
    This double processing can be time consuming for large projects and is especially noticeable when you have additional external tools running to modify files (like SASS). If you change a single SASS file, the SASS tool will run and regenerate all CSS files, and the first one to finish will trigger eleventy to rebuild, and then they will immediately re-process a second time after that.

This PR add support for a new config property watchTriggerDelay (open for alternate suggestions). If watchTriggerDelay is provided eleventy will wait until that long AFTER the last file change occurred before it starts reprocessing. This means that if you set watchTriggerDelay to 10 seconds, and save a file, and then wait 5 seconds and save a second file, then it will wait another 10 seconds before starting to process any changes.

As a side-effect of this change, we now also queue up any file changes that do occur while we're processing and will refresh all of them instead of just the very last one. This change does NOT attempt solve the fact that file changes during processing can result in unexpected output, but it will still trigger an immediate re-process immediately afterwards for any changes that didn't make it into the first "batch".

This change is using an async sleep method by the es7-sleep package, but it's an incredibly minimal implementation so if you'd prefer dropping that dependency we can move it directly into eleventy.

@zachleat
Copy link
Member

Hmm, can we solve this by extending/exposing a configuration API to set chokidar options? In situations like this I’d rather defer to exposing configuration rather than solving it with more code internally.

See:
https://github.com/paulmillr/chokidar#api

Maybe by adding an Object.assign to:
https://github.com/11ty/eleventy/blob/master/src/Eleventy.js#L364

This is similar to what we do with advanced BrowserSync usage here:
https://github.com/11ty/eleventy/blob/master/src/UserConfig.js#L494 and https://github.com/11ty/eleventy/blob/master/src/EleventyServe.js#L63

@zachleat
Copy link
Member

zachleat commented Jun 12, 2019

Actually now that I think about it we may already allow customization of this, because you can set watchOptions in browser sync config: https://browsersync.io/docs/options#option-watchOptions

Docs here: https://www.11ty.io/docs/config/#override-browsersync-server-options

@zachleat zachleat added the needs-discussion Please leave your opinion! This request is open for feedback from devs. label Jun 12, 2019
@veleek
Copy link
Contributor Author

veleek commented Jun 12, 2019

There may have been a bit of a miscommunication in the original issue. I actually already tried manually overriding the chokidar options on my own build but it doesn't solve the problem. The chokidar option I referred to (awaitWriteFinish) actually just holds off sending file change events until it's sure the file is no longer changing. It doesn't actually batch up any file change requests so eleventy still receives each one of them separately. The current behavior looks like this:

Delays for eleventy processing are show way lower than they actually are, but lets just say it's 1000ms.
And lets say it takes gulp about 50ms to process each file.

T(ms) |Gulp                 |Chokidar          |Eleventy (event)      |Eleventy processor
0     | Trigger on .scss    |                  |                      |
50    | regenerate all css  |                  |                      |
100   | Generate F1.css --> | Trigger F1 ----->| Trigger Rebuild  --> | Processing F1, F2 and F3
150   | Generate F2.css --> | Trigger F2 ----->| Queue Rebuild        |
200   | Generate F3.css --> | Trigger F3 ----->| Queue Rebuild        |
250   |                     |                  |                      |
300                                                                   |
...                                                                   |
1100                                                                  | Rebuild complete 
-                                                                     | Check if build queued (yup)
1100                                                                  | Processing F1, F2 and F3
...                                                                   |
2100                                                                  | Rebuild complete 
-                                                                     | Check if build queued (nope)
2100                                                                  | Ready to serve files

Now assuming we have a 200 ms delay configured and we have this change merged:

T(ms) |Gulp                 |Chokidar          |Eleventy (event)      |Eleventy processor
0     | Trigger on .scss    |                  |                      |
0     | regenerate all css  |                  |                      |
50    | Generate F1.css --> | Trigger F1 ----->| Queue Rebuild F1 --> | Delay rebuild
100   | Generate F2.css --> | Trigger F2 ----->| Queue Rebuild F2     | Delaying
150   | Generate F3.css --> | Trigger F3 ----->| Queue Rebuild F3     | Delaying
200   |                     |                  |                      | Delayed 200ms.  
250                                                                   | Last file was changed at 150ms.
300                                                                   | Delay is 200ms from then, so 350ms
350                                                                   | Processing F1, F2 and F3
1350                                                                  | Rebuild complete 
1350                                                                  | Check if build queued (nope)
1400                                                                  | Ready to serve files

So unfortunately, just passing some configuration to to chokidar doesn't solve the problem. We explicitly need the ability to delay processing for some period of time. I referenced this in the issue, but I think it's worth referring to it here too, but gulp supports a delay option when watching files and it defaults to a non-zero value for the exact same reason we need it here.

@Ryuno-Ki
Copy link
Contributor

Maybe by adding an Object.assign to:
https://github.com/11ty/eleventy/blob/master/src/Eleventy.js#L364

This is similar to what we do with advanced BrowserSync usage here:
https://github.com/11ty/eleventy/blob/master/src/UserConfig.js#L494 and https://github.com/11ty/eleventy/blob/master/src/EleventyServe.js#L63

Those links will be wrong in the future. @zachleat if you are on the site, hit y to get a canonical URL (which will stay permanent over time). More tips hidden behind ? :-)

@veleek
Copy link
Contributor Author

veleek commented Jun 17, 2019

@zachleat - Any further thoughts on this? I've been using this for a week now and while it's only one user it seems to be working pretty well for me.

@veleek
Copy link
Contributor Author

veleek commented Aug 13, 2019

Hey @zachleat. I'm still hoping to get this merged if possible. Please let me know if there's anything else that you'd like to consider and I'll try to make the change so we can get this closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Please leave your opinion! This request is open for feedback from devs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for a "cool-down" period before triggering a regeneration.
3 participants