Emit error on file not found with singular globs #41

callumacrae · 2015-03-14T22:29:04Z

callumacrae · 2015-03-14T22:29:40Z

test/main.js

+        should.fail();
+      });
+
+      // For some reason `end` isn't called unless `data` is first?


I don't know wtf is happening here, for some reason end is never called unless I add a listener to data (which is never called, too).

@callumacrae yeah the stream has to be put into a flowing state, they are paused by default. adding a data handler or calling .resume will do it for you. you can also pipe it somewhere, but resume is probably easiest for this test

coveralls · 2015-03-14T22:31:05Z

Coverage decreased (-1.25%) to 98.75% when pulling 4eacfeb on callumacrae:empty-error into 6411ea4 on wearefractal:master.

coveralls · 2015-03-14T22:38:57Z

Coverage remained the same at 100.0% when pulling 7c321c5 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

yocontra · 2015-03-14T22:58:11Z

index.js

+    if (positives.length === 1) {
+      return streamFromPositive(positives[0]);
+    } else {
+      opt.allowEmpty = true;


opt.allowEmpty should be true by default, probably best to use lodash.defaults

It is true by default, this is just stopping errors from being thrown when there's multiple paths in the array (see the "should not emit error when multiple globs not found" test).

I guess this could be considered an abuse of configuration, but it's certainly the easiest way to do it.

Uh, ignore what I just said. You said in the gulp issue that it should be false by default (so, it throws an error if an option isn't specified). Have you changed your mind?

@callumacrae Sorry, late night when I said that. false by default.

coveralls · 2015-03-14T23:04:20Z

Coverage remained the same at 100.0% when pulling 0216b07 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

yocontra · 2015-03-15T09:12:11Z

I think this PR was based on the assumption that it should only fail if one direct path is passed in, but it should also fail for

gulp.src(['a.js', 'b.js'])

if either a or b are not found

and also

gulp.src(['a.js', 'b*.js'])

if a.js is not found

yocontra · 2015-03-15T09:14:20Z

test/main.js

+      var stream = gs.create('notfound{a,b}');
+      should.exist(stream);
+      stream.on('error', function() {
+        should.fail();


for these tests i would usually do done('Error was emitted') that way if it does fail you have a more descriptive message

I'm going with throw new Error('Error was emitted');, because otherwise it just complains that done() is being called twice. I figure I could also just use _.once(done), but I don't see any advantage over this.

@callumacrae same effect, works for me

coveralls · 2015-03-15T12:33:54Z

Coverage remained the same at 100.0% when pulling e139649 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

coveralls · 2015-03-15T12:34:46Z

Coverage remained the same at 100.0% when pulling f9e7bd8 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

callumacrae · 2015-03-15T12:41:55Z

I think this PR was based on the assumption that it should only fail if one direct path is passed in, but it should also fail for

Would that behaviour be as intuitive?

It's also waay trickier to implement. I can't figure out how I would do it, because the match event doesn't say which glob was matched.

yocontra · 2015-03-15T12:48:52Z

@callumacrae Any direct path should fail if it wasn't found, only doing so when one is specified seems like the unintuitive route

yocontra · 2015-03-15T12:49:29Z

Also you do have access to the glob in match events https://github.com/callumacrae/glob-stream/blob/empty-error/index.js#L39 ourGlob is within scope

coveralls · 2015-03-15T23:14:34Z

Coverage remained the same at 100.0% when pulling 006467f on callumacrae:empty-error into 6411ea4 on wearefractal:master.

callumacrae · 2015-03-15T23:14:48Z

Done. Not sure if the weird error emitting stuff I'm doing is the best way, but I don't know any other.

I got confused by the whole create / createStream thing, I thought it would be trickier.

yocontra · 2015-03-16T08:04:07Z

index.js

@@ -95,8 +103,13 @@ var gs = {
    // then just pipe them to a single unique stream and return it
    var aggregate = new Combine(streams);
    var uniqueStream = unique('path');
+    var returnStream = aggregate.pipe(uniqueStream);
+
+    aggregate.on('error', function (err) {


errors should propagate downstream you shouldnt need to do this afaik

It seemed weird to me as well, but the tests fail without it: the error is just thrown instead of being emitted on the stream.

@callumacrae 🤷 as long as it works haha, we can revisit later

yocontra · 2015-03-16T08:12:41Z

Looks good, just need to tweak the error management and we are 👍

Emit error on file not found with singular globs

coveralls · 2015-03-16T15:33:55Z

Coverage remained the same at 100.0% when pulling e777792 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

coveralls · 2015-03-16T15:33:56Z

Coverage remained the same at 100.0% when pulling e777792 on callumacrae:empty-error into 6411ea4 on wearefractal:master.

armandabric · 2015-03-16T17:19:03Z

Nice PR 👍

phated · 2015-03-17T18:05:17Z

@contra this really should have been a 5.0 bump, not a 4.1

jameswomack · 2015-03-17T18:35:10Z

@phated Agreed. This changed build behavior here at Netflix.

callumacrae · 2015-03-17T18:48:02Z

Is it worth reverting it for 4.1.1 and releasing it in 5.0?

phated · 2015-03-17T18:50:48Z

@callumacrae i think so

yocontra · 2015-03-17T18:53:06Z

@phated typo'd the np command, you have access to the repo and pkg - would you mind doing the revert and republish? on a plane right now and i can barely load this webpage let alone npm publish

phated · 2015-03-17T23:05:41Z

@contra I have publish rights but not access to this repo

yocontra · 2015-03-19T01:49:14Z

Okay published as 5.0, which means now vinyl-fs needs to be bumped to 5.0 if it wants to depend on this and then gulp needs to be updated to depend on the new vinyl-fs

phated · 2015-03-19T03:15:10Z

@contra were you able to publish 4.1.1 that reverts the breaking change?

callumacrae · 2015-03-19T10:03:18Z

npm ERR! version not found: glob-stream@4.1.1

Doesn't look like it.

git checkout fac7fa8957867e7b1348261118b18185b09e5218^
git checkout -b 4.x
np
npm dist-tag add glob-stream@5.0.0 latest

should do the trick. You don't need to checkout a new branch if you don't use np.

yocontra · 2015-03-19T14:49:07Z

np totally fails when doing this, going to have to do it manually

yocontra · 2015-03-19T14:50:42Z

Also npm unpublish is totally failing. np just published a 5.1.0 with 4.x code and now I can't unpublish it. going to have to contact npm support because everything is fucked in this toolchain

callumacrae · 2015-03-19T14:51:02Z

You can't unpublish. Did you try what I said or did you start from 5.0? Because you can't do that.

yocontra · 2015-03-19T14:52:04Z

Okay all done. 4.1.1 === 4.0.1, 5.0.0 = new stuff, tagged as latest. For future reference adding --force to the end of anything makes npm work

callumacrae · 2015-03-19T14:52:17Z

👍

adamreisnz · 2015-11-04T06:26:48Z

Would it be possible to display the glob/filename that is causing the error? Right now it's very undescriptive and I have no idea which glob is causing the issue:

Error: File not found with singular glob
  at Glob.<anonymous> (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob-stream/index.js:34:30)
  at emitOne (events.js:77:13)
  at Glob.emit (events.js:169:7)
  at Glob._finish (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:172:8)
  at done (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:159:12)
  at Glob._processSimple2 (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:652:12)
  at /Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:640:10
  at Glob._stat2 (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:736:12)
  at lstatcb_ (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/glob/glob.js:728:12)
  at RES (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/inflight/inflight.js:23:14)
  at f (/Users/Adam/Sites/meanie/boilerplate/src/node_modules/once/once.js:17:25)
  at FSReqWrap.oncomplete (fs.js:82:15)

phated · 2015-11-04T06:30:55Z

This is master but we aren't able to cut a release yet.

Emit error on file not found with singular globs

callumacrae added 2 commits March 14, 2015 22:27

Added editorconfig

cc7660d

Emit error on file not found with singular globs

4eacfeb

callumacrae reviewed Mar 14, 2015
View reviewed changes

Added a test for {} in globs

7c321c5

yocontra reviewed Mar 14, 2015
View reviewed changes

inlined isString

0216b07

yocontra reviewed Mar 15, 2015
View reviewed changes

callumacrae added 2 commits March 15, 2015 12:29

Improved tests

e139649

Whoops, missed that resume.

f9e7bd8

Now errors if any singular blog isn't found

006467f

yocontra reviewed Mar 16, 2015
View reviewed changes

Added allowEmpty docs

e777792

yocontra pushed a commit that referenced this pull request Mar 16, 2015

Merge pull request #41 from callumacrae/empty-error

fac7fa8

Emit error on file not found with singular globs

yocontra merged commit fac7fa8 into gulpjs:master Mar 16, 2015

callumacrae deleted the empty-error branch March 16, 2015 17:42

yocontra mentioned this pull request Mar 31, 2015

Gulp.src fails for non existing files gulpjs/gulp#992

Closed

phated mentioned this pull request Oct 1, 2015

Is gulp.src too strict about input globs? (gulp 4) gulpjs/gulp#1298

Closed

phated pushed a commit that referenced this pull request Feb 16, 2017

Merge pull request #41 from callumacrae/empty-error

1895396

Emit error on file not found with singular globs

phated pushed a commit that referenced this pull request Feb 16, 2017

Merge pull request #41 from callumacrae/empty-error

f99e2a5

Emit error on file not found with singular globs

pelletiermaxime mentioned this pull request Aug 17, 2021

log error when no files found in stream dowjones/gulp-bundle-assets#13

Open

Emit error on file not found with singular globs #41

Emit error on file not found with singular globs #41

Conversation

callumacrae commented Mar 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 14, 2015

coveralls commented Mar 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 14, 2015

yocontra commented Mar 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 15, 2015

coveralls commented Mar 15, 2015

callumacrae commented Mar 15, 2015

yocontra commented Mar 15, 2015

yocontra commented Mar 15, 2015

coveralls commented Mar 15, 2015

callumacrae commented Mar 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yocontra commented Mar 16, 2015

coveralls commented Mar 16, 2015

coveralls commented Mar 16, 2015

armandabric commented Mar 16, 2015

phated commented Mar 17, 2015

jameswomack commented Mar 17, 2015

callumacrae commented Mar 17, 2015

phated commented Mar 17, 2015

yocontra commented Mar 17, 2015

phated commented Mar 17, 2015

yocontra commented Mar 19, 2015

phated commented Mar 19, 2015

callumacrae commented Mar 19, 2015

yocontra commented Mar 19, 2015

yocontra commented Mar 19, 2015

callumacrae commented Mar 19, 2015

yocontra commented Mar 19, 2015

callumacrae commented Mar 19, 2015

adamreisnz commented Nov 4, 2015

phated commented Nov 4, 2015