New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Direct path (non-glob) .src streams should fail on file not found #374
Comments
This could also probably apply to globs like I think this should be behind a flag because not everyone cares about this |
I can do this. Would listening for "nomatch" (edit: wrong library, whoops. Same idea, though) and making sure the glob is just a string that doesn't contain an asterisk be enough, for the first case? |
@callumacrae You should rely on the internal representation from node-glob to determine if it's a flat file or not. It will make an array of all path segments and each glob gets turned into a regex, so you can iterate the array and if there are no regexs then it can be assumed it's a flat file. |
Sample of me messing with the internal representation to find the base path for vinyl files https://github.com/wearefractal/glob2base/blob/master/index.js |
What is the purpose of having .src fail if a directory in a glob doesn't exist or a specific path (without wildcards) doesn't match? It seems inconsistent with the model of globbing returning all matching files -- 0 is still a valid number of matching files, and a pattern with no wildcards or that specifies a non-existent directory is still a pattern. What if your plugin is combining two sources, one may have no files and the other may have some (e.g. the using-multiple-sources recipe). You'd have to code around the error thrown from .src()... |
@stu-salsbury This is about flat paths, not globs. Example: |
This also brings up a good point performance wise. We shouldn't even be passing flat file paths to node-glob which then walks directories, we should just scoop them off the fs normally since we already have the path and know it isn't a glob. |
I understand that the paths don't have wildcards, but to my mind they're still globs... but aside from semantics, I think that when you have an array of files, some of which might not exist, you may still want the plugin (or other code) that's receiving the stream(s) to operate on the files that are there, if there are any, or to behave in a plugin/other-code-specific way rather than having to catch an error in the gulpfile. Also, in the case of the example in the recipe, this would just break: return streamqueue({ objectMode: true },
gulp.src('foo/*'),
gulp.src('bar/*')
) What is the reason for throwing an error if a file or directory isn't found? Is it solving a particular problem? |
@stu-salsbury No, that wouldn't break because those are globs not direct paths to files. |
@stu-salsbury Also FYI gulp.src maintains order now so you don't need to use streamqueue You could rewrite that example as |
I was under the impression that the scope of this is also directories that don't exist... but my point is in general.... what problem is being solved by throwing an error? If a plugin's purpose is to add files to a pipe, starting with zero is a perfectly reasonable place to start. Heck, the first plugin after .src might be one that is lazy-creating files that aren't present... I guess I'm not understanding the value in throwing an error. Why not just have a plugin whose purpose is to throw an error if it gets 0 files prior to the stream ending instead of jumping to the conclusion that we're always in an error state when a file or directory doesn't exist. |
@stu-salsbury It's misleading to have something like Also, disregard my thoughts about directories. |
I agree and I get your point -- I would definitely want that pipeline to inform me that 0 files were processed. Where I'm taking issue is that I think it is up to uglify to decide whether or not 0 files is ok, whether to log it, emit an error, throw an error, whatever... and perhaps .src, at a certain log level, should also log something about it. I think there's plenty of room between apparent success (neither .src nor uglify makes any mention that 0 files were processed) vs. just crashing a pipeline altogether. Perhaps an option to src might enable the new error throwing feature (or is it error emiting?) to be turned off? |
Basically, I don't want to be baby-sat into place where I have to write error handling code to accomplish my goal. Errors should be reserved for things that are definitely errors, not just things that are probably errors -- especially when you can't easily |
I guess I just figured out that I'd prefer to see a
and if the file being absent is a possible valid case, I could squelch the warning from my gulpfile. |
@stu-salsbury If added it would definitely be behind an option. I totally get your concerns. Also RE your comments about error handling being a pain in the ass - that is going to be fixed in gulp 4. One .on(error) handler or a domain will fix everything then |
I'd make the case that the default should in fact be to error. I think it is the exceptional case where it's ok that a hardcoded (non-wildcarded) file path points to a non-existent file. I was saying I think there should be an option in the second param to .src() to disable the error. |
Imo, the default behaviour should be to log "WARN: 'file.js' not found" to the console, and there should be a flag available (and an option to gulp.src) to make it throw an error instead of just logging a warning. Also, gulp.src should have an option to suppress the warning entirely. |
Any word on where this is going? |
Agreeing with @callumacrae on this. I am actually in favor of fail-fast system, so I would even consider to make it an error. My use case is when concatenating specific files. If I explicitly With the current situation my build breaks because the file |
Just got bit by this and it led me here. A task failed silently and it took hours to track down and realize nothing was happening in a gulp pipeline. The name |
How does |
As long as the default value is strict, it sounds good to me. |
I'd prefer it to be the default and have an |
Is this issue still alive ? I'm right now dealing with this case. For me options is a good proposal. With Maybe a third options to configure the error behaviour: |
As a temporary hack, you can fs.statSync the files before you gulp.src 'em:
|
This would be really nice to have. I like Spy-Seth's idea with the |
@datfinesoul, @Spy-Seth, I don't understand the meaning of the multiple options I believe a single option should be enough to control that behavior. Same goes for I would stick with original proposition of |
The But this is a must have. The important here is to have the |
I'm short on time right now but I will take a PR for this on the vinyl-fs repo if somebody wants to work on one |
+1 (when using complex build environment this would really help a lot) |
👍 |
Hi, any news about this? Wold be really helpful. |
At the moment I use simple check with var glob = require('glob');
// ...
// synchronously, because have to leave one message when all done and fine,
// or error messages for each mask
var files = glob.sync(mask);
if (!files || !files.length) {
// handle empty list
} But in this case have to copy-paste within projects, most of them have complex build and several file lists, which I actually have to store to check before build. |
+1 This would save a lot of time debugging. |
👍 |
Programmatically? Not too easily, you'd have to use through2 or another streams library. Visually, you can use gulp-debug and it will output the number of files passed through. |
@contra: I've got some time and want to have another crack at this, but I'm not sure how I would do it. Would I want to start by checking for nomatch below here, and then making a change to vinyl-fs once glob-stream has been changed, or is there an easier way to do it? I'm on IRC for the next hour or so if it's easier to discuss there. |
@callumacrae Yep check for nomatch in glob-stream and throw an error if the glob is not plural, unless an option is specified as false (default will be that this behavior occurs). This and https://github.com/wearefractal/glob-stream/blob/master/index.js#L30 should be a .once |
Also on IRC if you need a hand |
So this just got merged into glob-stream, going to publish and an npm update should have you on your way |
I'll send a PR to add docs to gulp in a bit |
@callumacrae's PR for documenting this was merged 6 days ago. The 4.0 branch pulls from vinyl-fs master currently (will update that to be published soon). Closing. Many thanks to @callumacrae for landing this. |
Example: https://gist.github.com/anonymous/9774298
If it isn't a glob it should probably fail if the file isn't found.
Reported by
heyladies
via IRCThe text was updated successfully, but these errors were encountered: