Ignore updates to source files while live-reloading is in progress #155 #31

rachx · 2018-02-24T09:42:34Z

Resolves MarkBind/markbind#155
Needs/ follows MarkBind/markbind#156

Attempt to address the problem that live reloading crash when there are

Multiple updates to a single file before live reloading is complete
- Testing: Before it logs 'Page rebuilt', continue making changes to file and save repeatedly.
Multiple files are saved at once (by the IDE)
- Testing: Use a IDE? or make multiple changes then use Git to discard changes in multiple files after markbind serve

Attempted fix - delay function:
Using rebuildSourceFiles as an example,

waitingPromise has not started executing anything in rebuildSourceFiles
runningPromise is still executing rebuildSourceFiles and its promises and is not fulfilled/rejected yet.

Since the args are accumulated, we only need 1 waitingPromise (hence the if (waitingPromise === null) check)

Before we can do func.apply(context, [pendingArgs]) e.g. rebuild multiple source files, we will have to

wait for the previous rebuild to complete (or live reloading might crash)
- Now, waitingPromise must wait for runningPromise to be fulfilled or rejected.
maybe wait for some time so we can batch process a few files together
- waitingPromise wait for Promise.delay(wait)
- This isn't strictly debouncing as it ensures at least k seconds after the first call is made (not the last as in debouncing)

Some examples with wait = 1 sec / further clarification

Changes are first made to File A. We do not rebuild the file immediately. We wait for 1 second and if File B and C change within this second, they will be rebuilt together.
If the ongoing rebuild is really slow (e.g. 2 sec), we will just collect all the subsequent file paths that need to be rebuilt and rebuild them together after the ongoing one is complete (the 1 sec wait was already complete). This is unlike the debounce + lock approach where they will still be done in batches decided by the debounced call.
Since the handlers in index.js expect to get a promise back and chain it, we return the same waitingPromise to all calls. If one file is missing which will cause ENOENT, multiple catch might be executed.

acjh · 2018-02-24T10:12:35Z

Every one second, rebuild all the files in the queue together and clear the queue.

Use lodash's debounce

"Every one second" is logically different from "debounce", which is closer to "after one second".

Rebuilding the affected file might take longer than 1 second. Hence to ensure that only one rebuild per file is called, use a lock/flag.

A lock does not ensure that only one rebuild per file is called; it ensures that one is run after another.

acjh · 2018-02-24T10:30:31Z

index.js

 const clear = require('clear');
 const fs = require('fs-extra-promise');
 const path = require('path');
-const Promise = require('bluebird');


Why remove this?

es-lint told me it was redundant 🙈

acjh · 2018-02-24T11:02:07Z

debouncedBuildAssets() calls buildAssets(), which calls buildMultipleAssets(filePaths) 😭

How about handling debounce in the existing Site.prototype.buildAsset(filePath)?
We might need https://github.com/bjoerge/debounce-promise.

acjh · 2018-02-25T03:42:23Z

It should be at any point in time, there is only one instance of a page rebuilding?

Yes.

acjh · 2018-03-07T02:56:11Z

lib/Site.js

+    if (waitingPromise === null) {
+      waitingPromise = new Promise((resolve, reject) => {
+        Promise.all([Promise.delay(wait), previousPromise]).then(() => {
+          previousPromise = waitingPromise || Promise.resolve();


This is in a block that checks if (waitingPromise === null).

Why don't we just return a chained Promise? Something like:

return waitingPromise.then(() => func.apply(context, pendingArgs));

Using rebuildSourceFiles as an example,

waitingPromise has not started executing anything in rebuildSourceFiles

pendingPromise is still executing rebuildSourceFiles and its promises and is not fulfilled/rejected yet.

Since the args are accumulated, we only need 1 waitingPromise (hence the if (waitingPromise === null) check)

Before we can do func.apply(context, pendingArgs) e.g. rebuild multiple source files, we will have to

wait for the previous rebuild to complete (or live reloading will crash)

This was previously done with async-lock

Now, waitingPromise must wait for pendingPromise to be fulfilled or rejected.

maybe wait for some time so we can batch process a few files together

waitingPromise wait for Promise.delay(wait)

This isn't strictly debouncing as it ensures at least k seconds after the first call is made (not the last as in debouncing)

Some examples with wait = 1 sec / further clarification

Changes are first made to File A. We do not rebuild the file immediately. We wait for 1 second and if File B and C change within this second, they will be rebuilt together.

If the ongoing rebuild is really slow (e.g. 2 sec), we will just collect all the subsequent file paths that need to be rebuilt and rebuild them together after the ongoing one is complete (the 1 sec wait was already complete). This is unlike the debounce + lock approach where they will still be done in batches decided by the debounced call.

Since the handlers in index.js expect to get a promise back and chain it, we return the same waitingPromise to all calls. If one file is missing which will cause ENOENT, multiple catch might be executed.

Thanks for the explanation!

I propose to remove the explicit Promise construction:

// Current waitingPromise = new Promise((resolve, reject) => { Promise.all([Promise.delay(wait), previousPromise]).then(() => { previousPromise = waitingPromise || Promise.resolve(); func.apply(context, pendingArgs).then(resolve, reject); waitingPromise = null; pendingArgs = []; }); }); // Proposed waitingPromise = Promise.all([Promise.delay(wait), previousPromise]) .then(() => { previousPromise = waitingPromise || Promise.resolve(); waitingPromise = null; let funcPromise = func.apply(context, pendingArgs); pendingArgs = []; return funcPromise; });

In the "Current" case, if previousPromise rejects, then waitingPromise will never resolve (nor reject).
That said, should it be finally instead of then? Otherwise, all of the calls after a reject will always fail.

Can we also rename previousPromise to runningPromise to better reflect the running/waiting state?
Otherwise, previousPromise = waitingPromise looks really odd.

rachx · 2018-03-07T02:58:04Z

index.js

      .then(() => {
        const watcher = chokidar.watch(rootFolder, {
-          ignored: [outputFolder, /(^|[/\\])\../],
+          ignored: [outputFolder, /(^|[/\\])\../, '/**/*.md__jb_tmp__'],


@nicholaschuayunzhi Can test this? I don't get this files when I use intellij or webstorm

@rachx, I'm still getting changes recorded.

Also do consider ignoring html tmp files

this worked for me:

ignored: [outputFolder, /(^|[/\\])\../, '*.md__jb_tmp__'],

acjh · 2018-03-08T05:57:53Z

lib/Site.js

+    if (waitingPromise === null) {
+      waitingPromise = new Promise((resolve, reject) => {
+        Promise.all([Promise.delay(wait), previousPromise]).then(() => {
+          previousPromise = waitingPromise || Promise.resolve();


Thanks for the explanation!

I propose to remove the explicit Promise construction:

// Current waitingPromise = new Promise((resolve, reject) => { Promise.all([Promise.delay(wait), previousPromise]).then(() => { previousPromise = waitingPromise || Promise.resolve(); func.apply(context, pendingArgs).then(resolve, reject); waitingPromise = null; pendingArgs = []; }); }); // Proposed waitingPromise = Promise.all([Promise.delay(wait), previousPromise]) .then(() => { previousPromise = waitingPromise || Promise.resolve(); waitingPromise = null; let funcPromise = func.apply(context, pendingArgs); pendingArgs = []; return funcPromise; });

In the "Current" case, if previousPromise rejects, then waitingPromise will never resolve (nor reject).
That said, should it be finally instead of then? Otherwise, all of the calls after a reject will always fail.

Can we also rename previousPromise to runningPromise to better reflect the running/waiting state?
Otherwise, previousPromise = waitingPromise looks really odd.

acjh · 2018-03-08T06:20:11Z

lib/Site.js


+/**
+* Creates a function that delays invoking func until after wait milliseconds have elapsed
+* and the previous function have resolved/rejected.


-* Creates a function that delays invoking func until after wait milliseconds have elapsed -* and the previous function have resolved/rejected. +* Creates a function that delays invoking `func` until after `wait` milliseconds have elapsed +* and the running `func` has resolved/rejected.

acjh · 2018-03-09T02:33:41Z

lib/Site.js


+/**
+* Creates a function that delays invoking `func` until after `wait` milliseconds have elapsed
+* and the running `func` have resolved/rejected.


"has resolved/rejected"

acjh · 2018-03-09T02:36:12Z

lib/Site.js

-Site.prototype.buildAsset = function (filePath) {
+/**
+ * Rebuild pages that are affected by change in filePath
+ * @param filePath path corresponding to file that have changed


This should use the same plural jsdoc, right?

Can we remove the jsdoc on _-prefixed versions, and add to non-prefixed versions?

acjh · 2018-03-09T02:37:14Z

lib/Site.js

+Site.prototype.rebuildSourceFiles
+  = delay(Site.prototype._rebuildSourceFiles, 1000);
+
+Site.prototype._buildMultipleAssets = function (filePaths) {


Can buildAssets() call _buildMultipleAssets(filePaths)?

acjh · 2018-03-09T08:07:27Z

lib/Site.js

+  const uniquePaths = _.uniq(filePaths);
+  const ignoreConfig = this.siteConfig.ignore || [];
+  const fileIgnore = ignore().add(ignoreConfig);
+  const relativeFiles = uniquePaths.map(filePath => path.relative(this.rootPath, filePath));


fileRelativePaths?

acjh · 2018-03-09T08:08:33Z

lib/Site.js

-    const fileRelative = path.relative(this.rootPath, filePath);
-    const fileToRemove = path.join(this.outputPath, fileRelative);
-    fs.removeAsync(fileToRemove)
+    Promise.all(removeFiles)


Remove explicit promise construction when using Promise.all.

Do the same in Site.prototype.buildAllAssets.

acjh · 2018-03-09T08:15:22Z

lib/Site.js

-        assets.map(asset => fs.copyAsync(path.join(this.rootPath, asset), path.join(this.outputPath, asset))),
-      )
-      .then(copyAssets => Promise.all(copyAssets))
+      .then(assetPaths => this._buildMultipleAssets(assetPaths))


Hmm, let's revert this if there's almost no benefit.

acjh · 2018-03-09T08:22:35Z

lib/Site.js

+/**
+* Creates a function that delays invoking `func` until after `wait` milliseconds have elapsed
+* and the running `func` has resolved/rejected.
+* @param func the promise-returning function to delay


func's parameter must be an array, right?

Nope but we can change it that way

Currently, it is using func.apply(context, pendingArgs); and pendingArgs is supposed to be the argument list. So all the delayed functions take in a variable number of arguments ...filePaths

Will you prefer it to be func.apply(context, [pendingArgs]); instead?

Function.prototype.apply() calls a function with arguments provided as an array.
The way before 55b874a is preferable, and already the case where func's parameter was an array.
I meant the jsdoc should specify that.

I don't understand why func.apply(context, [pendingArgs]); was suggested.

Example

function sum(a, b) { // does not take in an array return a + b; } function wrapsum(a, b) { // invalid without [] // this passes it as sum(a,b) instead of sum([a, b]) return sum.apply(this, [a, b]); }

If the delayed functions do not take in a variable number of arguments ...filePaths, if there is only one path, it will call rebuildSourceFile("filename") which won't work as an array is expected.

I forgot to use rest parameters for some (building assets) before 55b874a

That doesn't seem like a suitable example.

In our case, the wrapped function takes in a single argument — either a filename, or an array.

var _pendingArgs = []; var _sum = 0; function sum(a) { // does not take in an array _sum += a; return _sum; } function wrapsum(...args) { _pendingArgs.push(...args); // Fake delay if (_pendingArgs < 2) return -1; const pendingArgs = _pendingArgs; _pendingArgs = []; return sum.apply(this, [pendingArgs]); }

I mean, if the "original" function only expects only 1 number as above, this won't work:

wrapsum(1); // -1 wrapsum(2); // "01,2"

If its parameter is an array:

var _sum = 0; function sum(arr) { // takes in an array arr.forEach(a => { _sum += a }); return _sum; }

Then this will work:

wrapsum(1); // -1 wrapsum(2); // 3

Now I understand why func.apply(context, [pendingArgs]); was suggested.

Now I understand why func.apply(context, [pendingArgs]); was suggested.

I see that it's not necessary if rest parameters are used.

nicholaschuayunzhi · 2018-03-09T08:44:02Z

index.js

      .then(() => {
        const watcher = chokidar.watch(rootFolder, {
-          ignored: [outputFolder, /(^|[/\\])\../, '/**/*.md__jb_tmp__'],
+          ignored: [outputFolder, /(^|[/\\])\../, '*.md__jb_tmp__', '*.html__jb_tmp__'],


I'm really sorry, the previous suggestion that i mentioned didn't work - first time i tested i did not have any temp files built..

How about this instead:

ignored: [outputFolder, /(^|[/\\])\../, path => path.endsWith('___jb_tmp___')],

I've tested this multiple times and had no issue. Would this be better?

nicholaschuayunzhi · 2018-03-09T10:10:04Z

I've found another temp file.
Reload for file deletion: D:\NUS Year 2\SEM2\markbind\website-base\book\common\definitions.md___jb_old___

rachx · 2018-03-09T10:43:09Z

lib/Site.js

+
+  return function (arg) {
+    context = this;
+    pendingArgs.push(arg);


Also, for the non-prefixed method e.g. rebuildSourceFiles,
it should only take in one filePath parameter.
Currently, that is how it is used in index.js too.

If it is supposed to be multiple parameters
then the function should take in ...args, we should concat instead of push args or flatten the array in the delayed functions

As I commented above: The way before 55b874a is preferable.
The ~~spread syntax~~ rest parameters allowed the "wrapped" function to take in one or more parameters.
We should use concat since the "original" function accepted an array (seems clean).

rachx · 2018-03-10T07:09:37Z

@nicholaschuayunzhi @acjh updated to address all comments.
If this is okay, I will reorganise the commits

acjh · 2018-03-10T07:37:14Z

lib/Site.js

-  });
+/**
+ * Rebuild pages that are affected by change in filePath
+ * @param filePath path corresponding to file that have changed


acjh · 2018-03-10T07:39:49Z

lib/Site.js

+  const uniquePaths = _.uniq(filePaths);
+  const ignoreConfig = this.siteConfig.ignore || [];
+  const fileIgnore = ignore().add(ignoreConfig);
+  const relativeFiles = uniquePaths.map(filePath => path.relative(this.rootPath, filePath));


#31 (comment):

fileRelativePaths?

acjh · 2018-03-10T08:35:37Z

lib/Site.js

+* Creates a function that delays invoking `func` until after `wait` milliseconds have elapsed
+* and the running `func` has resolved/rejected.
+* @param func the promise-returning function to delay,
+*        func should be able to take in an indefinite amount of arguments


Continuing from #31 (comment):

aaron:
func's parameter must be an array, right?

rachx:
Nope but we can change it that way

Currently, it is using func.apply(context, pendingArgs); and pendingArgs is supposed to be the argument list. So all the delayed functions take in a variable number of arguments ...filePaths

Will you prefer it to be func.apply(context, [pendingArgs]); instead?

Can we require that the original func take in an array, and limit arg of the wrapped function to a single parameter? This doesn't seem like a good use case for spread syntax and rest parameters. For flexibility, we can add a check for whether arg is already an array or a single value.

Let's rename rebuildSourceFiles to rebuildAffectedSourceFiles to better reflect what it does (similar to the existing regenerateAffectedPages).

To clarify,

Can we require that the original func take in an array

Does this mean removing the rest parameters in rebuildSourceFiles etc and ensuring an array is always passed in, e.g. by using [pendingArgs] and updating the jsdocs accordingly?

limit arg of the wrapped function to a single parameter

Does this mean changing function (...args) [code] in delay to function (arg).

So arg in rebuildSourceFiles (the non-prefixed method) can be a single value or array

This doesn't seem like a good use case for spread syntax and rest parameters.

Which lines are you referring to?

return function (...args)

pendingArgs.push(...args); to extend an array

Site.prototype._buildMultipleAssets = function (...filePaths)

Does this mean removing the rest parameters in rebuildSourceFiles etc and ensuring an array is always passed in, e.g. by using [pendingArgs] and updating the jsdocs accordingly?

Yes.

Does this mean changing function (...args) [code] in delay to function (arg).

So arg in rebuildSourceFiles (the non-prefixed method) can be a single value or array

Yes.

Which lines are you referring to?

This as in our use case of passing in a single filePath for "original" function accepting an array.

is not useful because we really won't be calling it as buildAsset(filePath1, filePath2) even if we have multiple files. It makes more sense to pass in an array.

seems reasonable at first, but flattening defeats the purpose of rest parameters.
e.g. If we have an "original" function that accepts (param1, param2), pendingArgs becomes [param1_first, param2_first, param1_second, param2_second]. So we're already expecting the "original" function to collect it into an array via rest parameters.

is not useful for the same reason as 1.

nicholaschuayunzhi · 2018-03-11T16:57:32Z

I've tested abit and it works well with md files especially if its only content. The delay works well to queue up all other actions.

However when i delete files, i think the rebuild process does not work (or it takes too long due to all the logging) and when i readd the files, not all the files are rebuilt. I think this can be handled in a different PR.

rachx · 2018-03-12T02:26:56Z

However when i delete files, i think the rebuild process does not work (or it takes too long due to all the logging) and when i read the files, not all the files are rebuilt. I think this can be handled in a different PR.

Can you elaborate?

I have tested deleting files before - the page that includes those files did rebuild, fresh and show an error that the referenced page was missing.

However, the error was logged in the console multiple times

Also when I tested in the past, sometimes it seems to "hang" and will work when I type enter or something in the command line

nicholaschuayunzhi · 2018-03-12T07:52:49Z

When i delete book/common/definitions.md, its logs alot of missing files and I'm not sure when the rebuilding process will complete.

rachx · 2018-03-13T04:39:45Z

@acjh

acjh

Good work!

acjh · 2018-03-13T11:50:39Z

lib/Site.js


+/**
+ * Remove assets that are specified in filePaths
+ * @param filePaths a single path or an array of patsh corresponding to the assets to remove


Typo: ~~patsh~~paths

…s and until the previous promise has resolved

Gisonrg · 2018-03-17T09:06:50Z

lib/Site.js

+* @param wait the number of milliseconds to delay
+* @returns delayedFunc that takes in a single argument (either an array or a single value)
+*/
+function delay(func, wait) {


nit: this function should live under a utility file in ./util/ since it could be used in other places than Site.js.

acjh · 2018-03-18T15:18:24Z

@rachx You can merge this :)

acjh reviewed Feb 24, 2018

View reviewed changes

acjh reviewed Mar 7, 2018

View reviewed changes

rachx commented Mar 7, 2018

View reviewed changes

acjh reviewed Mar 8, 2018

View reviewed changes

acjh reviewed Mar 9, 2018

View reviewed changes

nicholaschuayunzhi reviewed Mar 9, 2018

View reviewed changes

rachx commented Mar 9, 2018

View reviewed changes

acjh reviewed Mar 10, 2018

View reviewed changes

rachx added 2 commits March 11, 2018 23:16

Add missingIncludeSrc in included files

23041d6

Ignore intellij temp files

4ed5808

rachx mentioned this pull request Mar 13, 2018

Improve resilience of live reloading to temp IDE files MarkBind/markbind#163

Closed

acjh added this to the v1.6.1 milestone Mar 13, 2018

acjh approved these changes Mar 13, 2018

View reviewed changes

Use custom delay function to delay rebuildSourceFiles etc. for wait m…

e37aba3

…s and until the previous promise has resolved

acjh requested a review from Gisonrg March 15, 2018 04:45

Gisonrg approved these changes Mar 17, 2018

View reviewed changes

Extract delay into utils

5f10462

rachx merged commit 737b146 into MarkBind:master Mar 18, 2018

acjh mentioned this pull request Aug 27, 2018

Add a --force-reload flag to markbind serve MarkBind/markbind#424

Merged

acjh mentioned this pull request Jan 14, 2020

Adopt ES6 class syntax MarkBind/markbind#979

Merged

Ignore updates to source files while live-reloading is in progress #155 #31

Ignore updates to source files while live-reloading is in progress #155 #31

Uh oh!

Conversation

rachx commented Feb 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acjh commented Feb 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acjh commented Feb 24, 2018

Uh oh!

acjh commented Feb 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachx Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaschuayunzhi Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaschuayunzhi commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acjh Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachx commented Mar 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachx Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

rachx commented Feb 24, 2018 •

edited

Loading

rachx Mar 10, 2018 •

edited

Loading

nicholaschuayunzhi Mar 9, 2018 •

edited

Loading

acjh Mar 9, 2018 •

edited

Loading

rachx Mar 10, 2018 •

edited

Loading

rachx commented Mar 12, 2018 •

edited

Loading