atticoos / gulp-ng-config

:wrench: Create AngularJS constants from a JSON config file
MIT License
173 stars 35 forks source link

Emit error on invalid environment option #40

Closed NickAb closed 9 years ago

NickAb commented 9 years ago

Fixes #39

I tried to write a unittest for this one, but I'm relativly new to node and staff, so there is some problem. Here is a code: expect(spy).to.have.been.called(); reports that it was not called, but if I replace .on('error', spy); with gutil.log, then I can see that gutil.log does get called.

    it('should emit an error if an environment key is supplied and the key does not exists', function () {
      var stream = gulp.src(path.normalize(__dirname + '/mocks/input_3.json')),
          spy = chai.spy();
      expect(function () {
        stream.pipe(plugin('gulp-ng-config', {
          environment: 'env'
        })).on('error', spy);
      }).to.not.throw(Error);
      expect(spy).to.have.been.called();
    });
atticoos commented 9 years ago

That is weird, you might need to do

function handler () {
  spy();
}
stream.pipe(plugin('gulp-ng-config', {
  environment: 'env'
})).on('error', handler);

The callback may become bound do a different context and do weird things.

You know, kinda like this scenario:

function Something() {
  this.foo = 'bar';
}

Something.prototype.handleAction = function () {
  console.log(this.foo);
};

var something = new Something();

eventSource.on('action', something.handleAction);
// this will console.log `undefined`
// versus:
eventSource.on('action', something.handleAction.bind(something))
// this will console.log `bar`
NickAb commented 9 years ago

I have added a working unittest, it solves the problem that I had with spy not being called. The problem was with expects not being called synchronously, so expect(spy) was called before pipe(plugin('gulp-ng-config' ... was able to complete. But there must be a better way to do this, than I did. Any help will be appreciated.

atticoos commented 9 years ago

Sure thing, I can try to take a look tomorrow or this week

atticoos commented 9 years ago

Sorry for the delay merging this. I really appreciate the contribution! I've taken care of the conflicts that had come up. This will be part of the next release :+1:

NickAb commented 9 years ago

Thanks