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

Gulp 4: gulp.series repeats error log #1330

Closed
aparajita opened this issue Oct 17, 2015 · 15 comments
Closed

Gulp 4: gulp.series repeats error log #1330

aparajita opened this issue Oct 17, 2015 · 15 comments

Comments

@aparajita
Copy link

When a task within gulp.series generates an error, the error is logged twice: once by the task that generated the error, and once by gulp.series. Is there some way to suppress this by default?

@phated
Copy link
Member

phated commented Oct 19, 2015

Nope. This is an artifact of us having to propagate any error that happens in the execution of the series but also logging the error when they occur (incase someone is using the --continue flag)

@aparajita
Copy link
Author

In the current architecture, is it possible to turn off propagation when we know there is no need to?

@phated
Copy link
Member

phated commented Oct 20, 2015

No

@phated phated modified the milestone: gulp 4 Nov 12, 2015
@adamreisnz
Copy link

This is actually quite bothersome. I have several nested tasks, in series and parallel, and some errors that I get are propagated and logged up to 4 or 5 times. I think Gulp should probably be catching those errors behind the scenes and output them only once.

@aparajita
Copy link
Author

@adambuczynski I agree. It's kind of useless to see the same error multiple times. There is no need for the series/parallel task to report an error, because by definition the actual task noticed the error and (hopefully) reported it.

@sttk
Copy link
Contributor

sttk commented Dec 10, 2015

The error object is taken over by upper tasks.
So the following example codes can suppress too much error logs.

(undertaker/lib/helpers/createExtensions.js)
function createExtensions(ee) {
  ...
  error: function(error, storage) {
    ...
    error.logged = true;   // Added
  }
};

(gulp-cli/lib/versioned/^4.0.0/log/events.js)
function logEvents(gulpInst) {
  ...
  gulpInst.on('error', function(e) {
    if (e.error.logged) { return; }   // Added
    ...
  });
}

or

(undertaker/lib/helpers/createExtensions.js)
function createExtensions(ee) {
  ...
  error: function(error, storage) {        
    if (error.logged) { return; }  // Added
    error.logged = true;           // Added
    ...
  }
};

or

(gulp-cli/lib/versioned/^4.0.0/log/events.js)
function logEvents(gulpInst) {
  ...
  gulpInst.on('error', function(e) {
    if (e.error.logged) { return; }   // Added
    e.error.logged = true;            // Added
    ...
  });
}

@phated
Copy link
Member

phated commented Dec 18, 2015

Implementation in latest referenced PR.

@aparajita
Copy link
Author

@phated and @sttk Wonderful, thank you! I knew it couldn't be that difficult.

@phated
Copy link
Member

phated commented Dec 18, 2015

@aparajita yep, just need to take time to think through the correct and best implementation so we don't ship technical debt

@sttk
Copy link
Contributor

sttk commented Dec 19, 2015

@phated Yes, I don't like the solution in my post above, too.
If there is another better way, I hope it will be adopted.

However, in now-and-later (next and handler functions in map.js and mapSeries.js), all extension's error processes of upper tasks are called when an error caused.

And the error object is the only object which is shared among those tasks.
If this issue need to be solved, the error object have to be used somehow.

@phated
Copy link
Member

phated commented Dec 21, 2015

@sttk I've already committed a change better than the example you provided. The only place this matters is in the CLI and the underlying libraries should not be changed in their behavior.

@sttk
Copy link
Contributor

sttk commented Dec 21, 2015

@phated OK. I didn't conceive any more solutions and I also think your implementation is better.
I agree with you.

@aparajita
Copy link
Author

Shouldn't this be closed?

@phated
Copy link
Member

phated commented Dec 21, 2015

@aparajita I'm still working on releasing gulp-cli version 1.1 - it will be closed after release.

@phated
Copy link
Member

phated commented Dec 21, 2015

gulp-cli v1.1.0 has been published with this change. You need to have the latest undertaker installed as a dependency of gulp to have everything working.

@phated phated closed this as completed Dec 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants