Correct gulp install script's wrench config for src/site merges

[slife]

Even though the gulp install script outputs:
Site folder exists, merging files (no overwrite) project-dir/semantic/src/site/

The src/site directory is blown away (not preserving existing files) due to the forceDelete:true setting.

[jlukic]

My understanding is that forceDelete is required to add new files to an existing directory, but preserveFiles: true makes sure that individual files are not overwritten.

See this big old note in WrenchJS readme

I'm worried that if we change forceDelete: false then we will not be able to add new element definitions into site/ in existing folders.

I will need to retest this.

[slife]

You're correct. This PR doesn't correct the problem completely. This fix preserves the files in the site/ dir but makes it so any new files aren't copied into site/ during the script's install task.

There seems to be a wrench-js issue copying into an existing directory while preserving existing files. The preserveFiles: true isn't read before it deletes the existing directory.

As you mentioned, in wrench v1.5.0 there was a breaking change introduced which seems to be impacting the install script. If forceDelete: true, it will delete the existing directory, otherwise it will return new Error(...). Because wrench has return new Error() instead of a throw new Error() and the install gulp script doesn't check the return for wrench.copyDirSyncRecursive(), this fails silently.

From: https://github.com/ryanmcgrath/wrench-js/blob/master/lib/wrench.js

exports.copyDirSyncRecursive = function(sourceDir, newDirLocation, opts) {
    opts = opts || {};

    try {
        if(fs.statSync(newDirLocation).isDirectory()) { 
            if(opts.forceDelete) {
            exports.rmdirSyncRecursive(newDirLocation);
            } else {
                return new Error('You are trying to delete a directory that already exists. Specify forceDelete in the opts argument to override this. Bailing~');
            }
        }
    } catch(e) { }

    //...

I'll submit a PR to wrench-js to honor the preserveFiles option and bypass the rmdirSyncRecursive() if it is set to true. This seems to be the best option.

What are your thoughts?

[slife]

PR created for correcting this behavior within wrench:
ryanmcgrath/wrench-js#100

[jlukic]

Thanks @derekslife you present this all quite lucidly.

I think we can point to your forked wrench copy as an NPM dependency, add your patch, then wait for a response to your PR. The project seems to be relatively idle, so I'm not counting on it being quick.

[jlukic]

Merged into next in 1ca71b0

[jlukic]

I've tested install with changes and everything seems to be fine, files are preserved and new files are copied to existing directories. Pushing with next.

[slife]

Thanks.

In the long term, due to wrench's stale project state, we should explore moving away from it all together. It would be more idiomatic to just use gulp's own vinyl-fs. However, we may have to submit a PR for this issue (gulpjs/vinyl-fs#30) to get the overwrite behavior needed.

[jlukic]

Agreed. Would prefer to use something native. Its always nice to replace a 10 liner with a 1 liner though when it makes sense. Lets pray Gulp 4 won't require everything to be rewritten.

[slife]

PR Submitted: gulpjs/vinyl-fs#59