Absolute paths in glob not working

[szymonkups]

There is an issue when using absolute paths as globs in gulp-watch.
Example:

var gulp = require('gulp'),
    watch = require('gulp-watch'),
    glob = '/home/username/tmp/client/js/**/*.js';

const opts = { };

gulp.task('stream', function () {
    return gulp.src(glob, opts)
        .pipe(watch(glob, opts))
        .pipe(gulp.dest('build'));
});

Gulpfile is located under: /home/username/tmp/gulpfile.js.
This was working without problems in 4.3.5, and was broken by this commit. After the change matching of absolute glob and relative file path is not correct and glob is always -1.
Please provide more information about why this check is needed in that situation.

Thanks for the great work on the plugin.

[UltCombo]

Hey there, thanks for reporting this issue.

As you may have noticed, the reasoning for that change was in #206 but it is no longer accessible due to the termination of the issue creator's account as per violation of GitHub's terms of service. (more info)

So, I'll have to reconstruct the reasoning from memory. I believe that commit addresses two issues:

1. When you pass a directory path to Chokidar (the filesystem watching library that gulp-watch uses), it implicitly recursively watches all the descendant files and directories. This is often unexpected as it deviates from gulp 3's gulp.watch behavior.
2. That commit also solves some crash problems, see Error when deleting a foder containing watched file #227. (IIRC not working path with "+" symbol C:\localhost\coffee++\src #206 had more detailed diagnosis, but oh well)

Now, onto what really matters. Although the first problem can be seen as a bug, there is certainly people depending on that behavior, so it should have been released as a breaking change (major version) instead of a patch.

I'm willing to revert to the old directory path behavior and release a new patch version, so that anyone depending on gulp-watch@^4 or ~4.3 get a backwards compatible version as expected. However, it is not as simple as just reverting that specific commit—that would reintroduce the crash regression (#227), and I hope you agree fixing a regression to introduce another one does not sound like a good plan.

So, I believe a good plan would be:

1. Revert that change. Investigate and fix the crash issue in a way that does not change the directory path behavior (passing a directory path watches all descendant files and directories).
2. Release a new patch version, restoring compatibility with packages that depend on gulp-watch 4.x.
3. Check how gulp 4 (which now uses Chokidar) handles this specific scenario (directory paths), and align with it accordingly. This is not entirely necessary, but I believe it would ease migrating from gulp.watch to gulp-watch. If this results in a behavior change, release a new major version.

What do you people think?
/cc @floatdrop @jonschlinkert and anyone who'd like to contribute here.

@szymonkups By the way, the solution you have suggested in the related issue seems to be the best, as it will work regardless of the direction we take here.

[jonschlinkert]

I read through everything, you've given a pretty thorough overview thanks. I'm still getting caught up but your plan sounds good. I also looked at the code suggested in ckeditor/ckeditor5#181 (comment) and that also makes sense.

[szymonkups]

Thanks for the fast answers and explanation.

Maybe this issue:

When you pass a directory path to Chokidar (the filesystem watching library that gulp-watch uses), it implicitly recursively watches all the descendant files and directories. This is often unexpected as it deviates from gulp 3's gulp.watch behavior.

can be resolved somehow by using chokidar's depth option? I just had a quick look on it's API and thought this might be a way to deal with this.

We will consider to use relative paths in our globs, thanks.

[UltCombo]

This went a bit off my radar, my bad.

Are you sure absolute paths are a problem? I believed the issue was just with implicitly watched directory contents. Can you please check whether removing the path.relative() from your sample would break it? I believe the /**/* at the end should be enough to fix the issue.

Our unit tests use absolute paths, so theoretically they shouldn't be a problem.

[UltCombo]

Can you check if #233 fixes the issue? Do a npm install UltCombo/gulp-watch#dir-depth to try my fix.

[szymonkups]

Are you sure absolute paths are a problem? I believed the issue was just with implicitly watched directory contents. Can you please check whether removing the path.relative() from your sample would break it? I believe the /*/ at the end should be enough to fix the issue.

My sample was just a way I think it could be fixed. Currently we are using:

const glob = path.join( dirPath, '@(src|tests)', '**', '*' );

(without path.relative()) and it is not working with v4.3.6. I've tried example from your branch and I got an error message:

wut? This shouldn't happen. Please open this link to report the issue:

These are details:

Globs: ["/home/szymon/Projects/cksource/ckeditor5/ckeditor5-engine/@(src|tests)/**/*"]
Filepath: ../ckeditor5-engine/src/datacontroller.js
Options:

{
  "base": "/home/szymon/Projects/cksource/ckeditor5",
  "nodir": true,
  "read": true,
  "buffer": true,
  "cwd": "/home/szymon/Projects/cksource/ckeditor5/ckeditor5",
  "dot": false,
  "silent": true,
  "nonull": false,
  "cwdbase": false,
  "events": [
    "add",
    "change",
    "unlink"
  ],
  "ignoreInitial": true,
  "readDelay": 10
}

[UltCombo]

Thanks for the feedback and testing! 😄

I may be short of time today but I'll take another look as soon as possible.

[UltCombo]

By the way, are you using the cwd option? Looks like that is what makes Chokidar emit relative file paths. Our globs are normalized to absolute paths, but the file paths emitted by Chokidar are not always absolute, so that is most likely why matching fails.

[UltCombo]

I've managed to reproduce the issue and fixed it! 🎉

@szymonkups Can you please try this last fix?

npm cache clean && npm install UltCombo/gulp-watch#dir-depth

[UltCombo]

Note my current patch has a relatively small breaking change: if you pass a cwd option without an explicit base, the emitted vinyl file's path will be relative to the inferred base instead of cwd. I think we could default base to cwd in order to preserve backwards compatibility. I'll check how gulp handles this later before merging the PR.

[szymonkups]

Thanks for working on this. I've used changes from #dir-depth branch and it seems that it is working correctly again 🎉
We are setting base option in this situation so it should be fine.

[UltCombo]

Yes, I just have to make sure that the implicit base behavior is correct to avoid another regression, and hopefully publish a new release tonight.

[UltCombo]

Gah, looks like the universe conspires to foil my plans. I should have time to finish working on this now.

There is just one remaining issue: with my proposed patch, the implicit base has changed in some cases. I believe this ideally should be treated as a breaking change, but I'm inclined to believe the new implicit base is a bug fix instead of a regression. The question is, is anyone actually depending on the old buggy behavior? If so, will the new behavior actually provide an improvement by fixing cases that they didn't cover or actually cause breakage?

To be clear, the new behavior establishes symmetry between a/b/** and cwd: 'a' + b/**, which I believe is what everyone would expect. Previously, the implicit base didn't make much sense when using the cwd option.

There are still issues with the implicit base behavior, mostly regarding implicit watching directory contents (depth) when a glob matches a directory. At least, these issues should be consistent independent of the cwd option now. In any case, the implicit base is not really dependable for most non-simple cases; this implicit complexity leads to refactoring hazards and I believe it was mostly a mistake in gulp's initial design. The ideal approach is to set an explicit base if you depend on the vinyl files' relative paths.

Rants apart, let's see what options we have:

1. Release the current proposed patch as a bug fix (patch). Pros: preserves the crash fix, restores support for absolute paths and implicit watching directory contents. Cons: the implicit base behavior change regarding the cwd option may be a breaking change.
2. Revert everything to the previous version and release it as a bug fix (patch). Pros: restores support for absolute paths and implicit watching directory contents. Cons: reintroduces the crash regression (Error when deleting a foder containing watched file #227).
3. Release the current proposed patch as a breaking change (major). Pros: same as point 1, but also the implicit base change is communicated as a breaking change. Cons: absolute paths and implicit watching directory contents stay broken in the latest version 4.x.x.

I believe the option 1 is the "less bad" approach. I'm mostly positive the implicit base changes can be seen as an improvement. Implicit bases have never been reliable for several cases; if the changes turn out to be a problem you can (and should!) set an explicit base to ensure that the relative paths are what you really expect.

I'll reinforce again that these implicit base changes affect only certain edge cases that were quite unreliable to begin with. In any case, if it turns out to be a bigger problem than I'm anticipating, we can consider reverting the old behavior for the sake of backwards compatibility, although I believe the amount of effort to restore the buggy behavior would not be nearly worth it—from the user side, if the stars align and you happen to be affected by the implicit base changes, the simple fix (and recommended approach) is to just set an explicit base option.

[UltCombo]

Fixed by #233.

[UltCombo]

Published [email protected]! 🎉