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

handle orchestration aborted events #97

Merged
merged 5 commits into from Sep 19, 2017
Merged

handle orchestration aborted events #97

merged 5 commits into from Sep 19, 2017

Conversation

memoryhole
Copy link
Contributor

@memoryhole memoryhole commented Sep 18, 2017

If running under a watcher, run-sequence will fail to trigger on subsequent calls to gulp.start when gulp.stop is called mid-sequence unless it handles the 'orchestration aborted' error.

Copy link
Owner

@OverZealous OverZealous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great find, thanks for digging into the codebase and not only finding the issue, but also providing a PR to fix it!

I have a few comments below, mostly about code style changes, but also there appears to be an unused variable.

If you can get those changes in, I'll get this merged and published today.

index.js Outdated
@@ -67,8 +69,11 @@ function runSequence(gulp) {
}

function finish(e) {
if (finished) return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished appears to never be set.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please match the style of this codebase, no spaces between language keywords (if, catch, etc) and the parentheses.

index.js Outdated
@@ -96,6 +101,12 @@ function runSequence(gulp) {
}
}

function onGulpError(e) {
if (e.message === 'orchestration aborted') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove space

test/main.js Outdated
@@ -260,6 +260,40 @@ describe('runSequence', function() {

called.should.eql(true);
})

it('should pass error if gulp execution halted in second execution', function(done) {
const stopTask = gulp.task('stopTask', function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure backwards compatibility, since this codebase is older, let's leave this as a var. (I struggle with it too, since I've been writing const and let for quite a while now.)

test/main.js Outdated

it('should pass error if gulp execution halted in second execution', function(done) {
const stopTask = gulp.task('stopTask', function() {
if (stopTask.shouldStop) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's several spaces between the keywords and parentheses in here.

@memoryhole
Copy link
Contributor Author

Thank you for the quick review! I'll update according your comments.

@OverZealous
Copy link
Owner

Looks great, I'll get this published.

@OverZealous OverZealous merged commit 2155560 into OverZealous:master Sep 19, 2017
@OverZealous
Copy link
Owner

OK, should be published as v2.2.0. (I'm hoping this is a non-breaking change for anyone!)

@memoryhole
Copy link
Contributor Author

@OverZealous should be, 🤞

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

Successfully merging this pull request may close these issues.

None yet

3 participants