Add a --force-reload flag to markbind serve

[jamos-tay]

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes #403

What is the rationale for this request?

Changing certain files will not trigger a rebuild. User might want a way to force a rebuild of all files.

What changes did you make? (Give an overview)

Add a --force-reload flag to markbind serve to allow user to rebuild all files whenever a file is changed.

When set, any change detected in the directory will immediately trigger a rebuild of all files.

[acjh]

Complications regarding rest parameters:

• Ignore updates to source files while live-reloading is in progress #155 markbind-cli#31 (comment)
• Ignore updates to source files while live-reloading is in progress #155 markbind-cli#31 (comment)

[jamos-tay]

Hi @acjh, I've looked through the two comment threads and sorry, but could you explain a little more about what changes I should make?

My rationale for changing the delay function was to add an extra parameter buildAsset(filepaths, forceReload), which was giving me problems as delay only allows one array parameter.

What I gathered from the first thread was that the old definition of delay adds a [] so it converts single arguments arg to an array:

  return function (arg) {
    
    // arg is a single value "test.html"
    
    context = this;
    if (Array.isArray(arg)) {
      pendingArgs = pendingArgs.concat(arg);
    } else {
      pendingArgs.push(arg);
    }
    
    // pendingArgs contains ["test.html"]
    
    ...

    const funcPromise = func.apply(context, [pendingArgs]); 
    // => func.apply(context, [["test.html"]])
    // => func(["test.html"]) is called instead of func("test.html")

Which is necessary for cases like buildAsset("filepath") -> buildAsset(["filepath"]).

But this seems strange to me because I expected delay to be a wrapper function that can be used with any func, it shouldn't be dependent on func's definition. Is there any reason that we should enforce delay to only work with functions that take in one array?

Regarding the use of ...args, the purpose was just to allow variable number of parameters for func. It should still work even if func doesn't use rest parameters.

For example:

function buildAsset(filepaths, forceReload) { // Not using rest
   ...
}

delayedbuildAsset = delay(buildAsset, 1000);

/*
    delayedbuildAsset is set to function(...args){
        ...
        buildAsset.apply(args);
    }
*/

delayedbuildAsset(["some filepath"], true); // Expect to call buildAsset(["some filepath"], true)

/*
    function(...args){
        // args is set to [["some filepath"], true]
        buildAsset.apply(args);
        // => buildAsset.apply([["some filepath"], true])
        // => calls buildAsset(["some filepath"], true) correctly
    }
*/

[acjh]

If I recall correctly, your change would cause this:

pendingArgs = [];
// ...
pendingArgs = [filePaths, forceReload];
// ...
pendingArgs = [filePaths, forceReload, filePaths, forceReload];

Did you test?

[jamos-tay]

Ah I see... I didn't know delay aggregated the params and executed them all at once, I thought it was just a simple delay function, my bad.

I moved the forceReload param to the Site constructor, I think that might be simpler for now.

[acjh]

How about a -f short flag? If yes, move above -p, else move above --no-open.

[acjh]

"forcesforce a full reload ..." — let's use imperative mood.

[acjh]

Alphabetical order.

[acjh]

• wrapped seems like an odd way to describe an "ensuring an array", but I don't have a better name at the moment.
• We should not lose the capital P for consistency.

[jamos-tay]

How about just filePathArray?

[acjh]

Ah I see... I didn't know delay aggregated the params and executed them all at once, I thought it was just a simple delay function, my bad.

It was named after Promise.delay, but we can rename if there is a better name used by other libraries.

• Allow delay to support multiple arguments

Remember to update PR description.

[jamos-tay]

I see, thanks for the information

[damithc]

@jamos-tay this feature doesn't seem to be reloading nunjucks code though :-(