Skip to content
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

doneCallback is persisted and called after other tasks with no callback #71

Open
jsocol opened this issue Jan 29, 2016 · 5 comments
Open

Comments

@jsocol
Copy link

jsocol commented Jan 29, 2016

We ran into this problem writing tests for Gulp tasks, but here's an example that demonstrates the problem with only Orchestrator:

'use strict';

var Orchestrator = require('orchestrator');
var orchestrator = new Orchestrator();

orchestrator.add('foo', function() {
  // Doesn't matter.
});

orchestrator.start('foo', function doneCB(err) {
  console.log('done with foo');
});

orchestrator.start('foo');

When run, the output is

$ node example.js
done with foo
done with foo

(The problem we had was using a jasmine done() call in the doneCallback, which would end up getting called multiple times, e.g.

describe('foo', function() {
  it('should do', function(done) {
    gulp.start('foo-task', function fooCB() {
      done();
    });
  });
  it('does another thing', function() {
    gulp.start('bar-task');  // fooCB gets called again, maybe, later
  });
});

)

Since Orchestrator already inherits from EventEmitter, doneCallback could be replaced with .once() and an internal event name, e.g.:

// index.js L79
this.once('::complete', lastTask);

// index.js L150
this.emit('::complete', err);

If that's something you'd be OK with, I'd be happy to open a PR.

@robrich
Copy link
Owner

robrich commented Jan 31, 2016

Thanks for the detailed description. This is a duplicate of #15.

@jsocol
Copy link
Author

jsocol commented Feb 1, 2016

@robrich So what's the deal with this project? Is it maintained, or is the answer to issues here "wait for Gulp 4"?

#15 is 2 years old, and closed, and references all sorts of other things that are closed (gulp#347, gulp#55) and have been for a while, but I reproduced this issue on master. I also reproduced it on gulp#master:

'use strict';

var gulp = require('gulp');

gulp.task('foo', function() {
  // Doesn't matter.
});

gulp.start('foo', function doneCB(err) {
  console.log('done with foo');
});

gulp.start('foo');

but not on gulp#4.0, because gulp lost its .start() method (which is a thing for another day).

@jsocol
Copy link
Author

jsocol commented Feb 1, 2016

OK, I opened #72 to add that to the README so folks don't go digging here.

@jsocol jsocol closed this as completed Feb 1, 2016
@robrich
Copy link
Owner

robrich commented Feb 2, 2016

@jsocol: gulp has abandoned orchestrator, but I have not. Orchestrator is now somewhat in a state of flux. It was built for Node 0.8 and 0.10, a world which no longer exists. Do we reboot it as https://github.com/orchestrator/orchestrator/tree/develop has done? Do we call it done and let it go? I've been pondering these thoughts for quite some time, and haven't come to a good conclusion yet. What do you think?

@robrich robrich reopened this Feb 2, 2016
@robrich
Copy link
Owner

robrich commented Feb 2, 2016

It seems I may have jumped the shark with this one. Created #73 to explore it more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants