Add support for passthrough with directory remapping

[MadeByMike]

Modifies the addPassthroughCopy function so that it accepts a 2nd param that allows you to remap the output directory.

This means you can do something like the following:

config.addPassthroughCopy({"src/site/static", "/"});

This will map all files and folders to the root directory.

After reading this issue I've updated it so that you can use globs. This will map all js files to the root directory:

config.addPassthroughCopy({"src/site/static/*.js": "/"});

Because of the way that globs work, it's not possible to retain nested directory structures and all matched files will be output to the specified directory. For example, this will find all file in nested folders named js and will output them to the 'js` folder in the root:

config.addPassthroughCopy({"src/site/static/**/js/*": "/js"});

This means if there are multiple index.js files in these folders they will be overwritten by the last copy.

To get around this and keep the option to recursively copy directories if the inputPath is not a glob and matches a directory, the directory will be recursively copied to the output path. For example, here all files in src/static/js will be copied to the js folder in root including any all sub directories:

config.addPassthroughCopy({"src/site/static/js": "/js"});

For backward compatibility passing true also works, and the output would be resolved to static/js:

config.addPassthroughCopy({"src/site/static/js": true});

Not passing an object has the same result:

config.addPassthroughCopy("src/site/static/js");

[edwardhorsford]

@MadeByMike this issue is possibly relevant to you.

[MadeByMike]

Thanks @edwardhorsford it's pretty close to what is suggested there maybe we can build on this.

[julrichkieffer]

👍 can't wait for this enhancement!

[kleinfreund]

Can you add tests for paths that escape the input or output directory, please?

config.addPassthroughCopy({
  "static": "..", // escapes output directory
  "..": "." // escapes input directory
});

They should throw an error.

Also, I’d prefer if no examples or tests use a leading forward slash as it usually represents the file system’s root directory. The tests don’t seem to do that, but you use it in your comments. I think it could be confusing and lead users to believe that they can target the root directory.

And finally, what does the following do? I don’t understand why there would be a boolean.

config.addPassthroughCopy({
  "static": true,
});

[MadeByMike]

I don’t understand why there would be a boolean

@kleinfreund It's how the old API worked. I would not expect anyone to do this, but that's what happens behind the scenes when you do config.addPassthroughCopy("static"). I needed to make sure it still works.

[jgarber623]

@MadeByMike Thank you so much for implementing this feature!

Quick question regarding:

Because of the way that globs work, it's not possible to retain nested directory structures and all matched files will be output to the specified directory.

(emphasis mine)

Could you explain that a bit more? Is this a path resolution issue with the way Node/JavaScript handles globbing?

For reference, I'm coming from the Ruby world where code like Dir.glob('./src/_assets/stylesheets/**/*.scss') would return an unsorted array with matched paths prefixed with the original query preceding the globbed characters (e.g. ./src/_assets/stylesheets/):

[
  "./src/_assets/stylesheets/application.scss", 
  "./src/_assets/stylesheets/components/_page-header.scss", 
  "./src/_assets/stylesheets/components/_page-footer.scss", 
  "./src/_assets/stylesheets/components/_buttons.scss", 
  "./src/_assets/stylesheets/components/_page-navigation.scss", 
  "./src/_assets/stylesheets/components/_hero.scss", 
  "./src/_assets/stylesheets/base/_variables.scss", 
  "./src/_assets/stylesheets/base/_utilities.scss", 
  "./src/_assets/stylesheets/base/_elements.scss"
]

[MadeByMike]

@jgarber623 yes exactly right! So imagine our key was the glob you mentioned and the output folder was ./assets:

{ './src/_assets/stylesheets/**/*.scss': './assets' }

We need to make a decision about what this means because it is ambiguous and what we choose will impose some limitations on users. My assumption is to match all .scss files and move them to the ./assets folder. (This is what has been implemented)

However you could also assume that the user intends to copy all .scss files from ./src/_assets/stylesheets/ and then copy these to ./assets keeping the folders in the stylesheets directory. This is easy for a human to understand but not so easy to find a clear set of rules that work with all globs.

If we match globs and then map files to a single directory, this means you only get the last file. If the match returns 2 files with the same name. Eg:

// If the glob result was:
[
"./src/_assets/stylesheets/base/index.scss"
"./src/_assets/stylesheets/typography/index.scss"
] 
// The output is:
[
"./assets/index.css" // From typography
] 

If it's not a glob we can recursively copy the directory.

// Result is:
[
"./src/_assets/stylesheets/"
] 
// The output is:
[
  "./src/_assets/stylesheets/application.scss", 
  "./src/_assets/stylesheets/components/_page-header.scss", 
  "./src/_assets/stylesheets/components/_page-footer.scss", 
  "./src/_assets/stylesheets/components/_buttons.scss", 
  "./src/_assets/stylesheets/components/_page-navigation.scss", 
  "./src/_assets/stylesheets/components/_hero.scss", 
  "./src/_assets/stylesheets/base/_variables.scss", 
  "./src/_assets/stylesheets/base/_utilities.scss", 
  "./src/_assets/stylesheets/base/_elements.scss"
]
// Along with any other non .scss files!

So it's that 3rd option of being able to recursively copy a directory and filter the results by regex that is tricky. I suggest that it should not be in scope for this PR but maybe we make a new issue to resolve it immediately after merging this?

[MadeByMike]

@kleinfreund I wrote a test and updated code to catch files attempting to be written outside the site output directory but I can figure out how to silence the errors in the console. If you'd like to help with this I'd love feedback.

@zachleat I'd love your input on this one too!

[MadeByMike]

I'm kinda of the opinion that anything that returns a promise should always return a promise and you reject rather than throw. This gives a better call stack in the error messages and makes it easier to catch on Promise.all().

I dunno I'm sure there are people with better opinions on this and who can articulate it better.

[kleinfreund]

I’m not sure why the errors are still thrown. Something is broken.

The example test in ava Assertions: .throwsAsync(thrower, [expected, [message]]) copied into the tests doesn’t spill its error out like that so that’s something on our end. The following works as expected:

test('throws', async t => {
	await t.throwsAsync(async () => {
		throw new TypeError('🦄');
	}, {instanceOf: TypeError, message: '🦄'});
});

Another note: Some of your tests have duplicate entries in the passthroughCopies object (e.g. "../": true is overwritten by "../": "./"). Please replace one of these instances with a different input path so both are actually tested.

And a question: What issue is commit 96fbd65 fixing? I don’t think we should normalize Windows path separators anywhere in the code.

[zachleat]

One of the things that TemplatePassthrough write did before was to return an unresolved promise. I believe this is waiting for the promises to resolve before returning, right?

[MadeByMike]

Yes, and to be honest I'm not sure what the best way to handle it is hence my comments here and here. @zachleat any chance you can give me some implementation advice?

[zachleat]

After a bit more review it looks like this case only hits when you’re using glob paths right? It keeps the previous behavior with files or directory. I think that’s okay.

[zachleat]

Hey! I fixed the conflicts here and am going to merge this but as a follow up I am opening an issue for the last remaining piece of feedback here. #615

[MadeByMike]

OMG OMG MERGED 😂😂😂

[zachleat]

Shipping with the next version!