feat: include only some files

[AndersDJohnson]

Adds a function include exported that you can call after importing which will limit the files that are considered for changes, and require all of them and only them to be present before reloading. Implementing it this way makes it backward compatible and non-intrusive.

I found this helpful in my extension where I was manually copying manifest.json after webpack build completed - it didn't exist right away when other webpack entry files ae written, which causes an extension reload error.

I partially figured this out anyway by enabling the copyUnmodified: true option with the CopyPlugin and using webpack --watch for development, but it still fails on a fresh start of webpack --watch, so I think this addition might be useful anyway.

const { include } = require("crx-hotreload");
include(["manifest.json"]);

Fixes #7, except does not support exclude.

[xpl]

In what cases timestamp is nonexistent?

[AndersDJohnson]

@xpl Since we generate timestamp from the set of files filtered by includes, if none of the included files exist, it may end up an empty string ('' from joining []), which is falsy. In this case, since none of the required files exist, we want to execute the timeout again, to wait for them to exist before reloading.

[xpl]

Can you please clarify what that line does?

[AndersDJohnson]

@xpl It checks if the user has specified any required includes, and if so, checks if not all of the files specified for inclusion actually exist in this timeout cycle - if any of the specified includes don't exist, it short-circuits so that we will wait for the next cycle to check again, hoping all the required files will exist then.

[xpl]

@AndersDJohnson Thanks for the PR! Sorry for the stupid questions, just don't have the time neither to check if it's working, nor for reviewing the code thoroughly (

In particular, it is not clear whether that line is necessary, as it seems redundant to me and should be removed:

if (includes.length && !includes.every(i => files.some(f => i === f.name))) return;

If there are no files is in includes, then won't that expression simply return an empty string:

return files
                .filter(f => includes.length ? includes.includes(f.name) : true)
                .map (f => f.name + f.lastModifiedDate).join ()

Also, I am not sure that it is right to do that thing:

 if (!timestamp || !lastTimestamp || (lastTimestamp === timestamp)) {

I suggest to change it to:

 if ((lastTimestamp === undefined) || (lastTimestamp === timestamp)) {

Because the purpose of the previous !lastTimestamp expr was to check if we are executing it the very first time (when the lastTimestamp is undefined). But that non-strict check is not good if we are now returning empty strings from timestampForFilesInDirectory, which is a valid value for a timestamp (if there are no files).

I haven't checked anything I've just written, so pardon for my silliness in advance :)

[AndersDJohnson]

@xpl I'm not sure if it's written correctly or minimally, but I think the !includes.every is ensuring that all the included files exist in the current list of files before generating a truthy timestamp that will cause a reload, since we should require all the included files to exist before reloading - whereas the .filter just removes any files that were not explicitly included from the timestamp (so as not to cause reloads when changing or being created or deleted), but if multiple files were included, the .filter may still allow through some that do currently exist, so that we may still get a truthy timestamp, while leaving out others included that aren't guaranteed to exist at this stage in the timeout loop.

[xpl]

@AndersDJohnson Thanks for the explanation. I somehow missed your initial premise that you need to ensure that all those files should actually exist before reloading, and thought that you simply want to implement a filtering for the file list needs to be watched, so I was confused of that additional checks' meaning.

Then I need to understand better the actual use case of that. Is it only for manifest.json or there could be issues with other files? Because if it's only because of manifest.json then maybe it is simpler to just hardcode the check for that file — as it seems that an extension cannot be loaded without that file anyway?

[AndersDJohnson]

@xpl Hard-coding for manifest.json could solve some use cases, and I'd be fine if we at least hard-coded that. But anyone else with a multi-step build process (e.g., generating different sizes of logos with a separate tool than the one that builds your JS) may experience similar issues where the extension reloads before they want it to do so.

[xpl]

@AndersDJohnson

anyone else with a multi-step build process (e.g., generating different sizes of logos with a separate tool than the one that builds your JS) may experience similar issues where the extension reloads before they want it to do so.

But then they would need to enumerate all those generated files explicity in includes, which could be painful, right? Because if there are multiple generated files, then they might get changed not simultaneously, and then the extension might get reloaded too early anyway?

That would work if a build tool first erases all files and then re-emits them — then your includes would ensure that the extension gets reloaded only after all the files are back. But what if a build tool just overwrites the files inplace sequentially? Then the reloading might occur before the build completes.

Do you have any information on how that is usually executed by various build tools?

[AndersDJohnson]

Yes for this to work for those use cases, I was assuming the files would be erased first, such as with the CleanWebpackPlugin.

I wonder if there is another way?

[xpl]

@AndersDJohnson

I wonder if there is another way?

We could implement some debouncing logic (like in _.debounce from lodash), postponing the reloading for 0.5-1s when a change detected — until files stop changing. That won't help if your build tool is painfully slow (changing files with larger pauses inbetween changes), but maybe will help in most cases.

The downside is that it would complicate the implementation and will also hurt the UX, as the reaction time of the reloading trigger would be slower, you would need to wait an additional second every time...

[chfabbro]

I created a simple version for myself that only uses a local exclude list (in hot-reload.js itself). Seems to work okay!

const ignoreList = [
  'popup.js',
  'popup-controller.js',
  'popup.html',
  'popup.css',
  'other',
];

// get the list of all files in the extension directory (recursively)
const filesInDirectory = dir => new Promise(resolve => dir.createReader().readEntries(entries => Promise.all(entries.filter(e => (e.name[0] !== '.' && !ignoreList.includes(e.name[0]))).map(e => (e.isDirectory
  ? filesInDirectory(e)
  : new Promise(res => e.file(res)))))
  .then((files) => {
    const myFiles = files.filter(f => !ignoreList.includes(f.name));
    return [].concat(...myFiles);
  })
  .then(resolve)));