Callback on error instead of process.exit when Shifter is called programmatically #115

counterflow · 2014-02-18T21:24:18Z

Added callbacks when handling errors. Removed the log.error call on the compressor callback to continue processing. This is to ensure that if Shifter is called programmatically and an error happens, a callback is executed rather than process.exit(1).

Submitting this for early review of the changes.

Still need to write unit tests.
Documentation

caridy · 2014-02-21T19:59:35Z

@ianstigator I will look into this PR soon, but for the note about process.exit(1), this is important for CI to fails properly when something fails during the build process.

unkillbob · 2014-02-21T20:35:50Z

@caridy the intention is for the behaviour to remain the same when called via command line but to rather callback with an error if called programmatically. This lets the calling code (e.g. a grunt task) decide whether to exit or not (e.g. don't exit when called from grunt watch).

counterflow · 2014-02-26T21:14:11Z

@caridy All tasks have been completed. No pending actions to do. This is now ready for a review.

caridy · 2014-02-26T21:19:17Z

lib/log.js

@@ -57,11 +57,15 @@ exports.warn = function (str) {
    }
 };

-exports.error = function (str) {
+exports.error = function (str, callback) {


this is the part that I don't like. log.error() has nothing to do with the callback, and the api of the callback function.

caridy · 2014-02-26T21:21:54Z

I want to encourage you to rethink the error callback logic since log.error() has nothing to do with the async nature of the callback api.

unkillbob · 2014-03-04T19:03:56Z

@caridy thanks for the feedback, done some thought on this and I propose:

everywhere that log.error() was used should log an error (just using log.err()) and callback (to their immediate caller) with an error
all callbacks eventually feed back to the initial call to shifter.init()
if that initial call to shifter.init() was given a callback, then ultimately that callback is called with the relevant error
if the initial call to shifter.init() was given no callback (i.e. called via the bin file) then shifter will process.exit(1) at that point.

Does that sound like a cleaner approach (rather than passing the callback to log.error())?

caridy · 2014-03-10T01:00:45Z

hey @unkillbob, @ianstigator, I haven't forgot about this, will get back to it on monday.

caridy · 2014-03-11T16:36:45Z

@unkillbob yes, that sounds reasonable. I fixed something related to this yesterday as well: #117, which was simply that the task was not calling back. I wonder how can the issue described here be reproduced.

unkillbob · 2014-03-11T18:52:41Z

@caridy the primary cause of this issue for us are catastrophic syntax errors in our JavaScript, shifter process.exits and so our grunt watch is killed. Often the developer immediately realises their mistake (I personally have jshint in my editor which points it out to me) and fixes it but doesn't realise that in the background the grunt watch has been killed and their changes aren't being built.

caridy · 2014-03-11T20:15:51Z

Ok @unkillbob, I was able to reproduce that. Alright, let's get this PR clean up.

…compressor callback to continue processing.

…cumentation

…and line and rollups. Improved the documentation.

…the initial call to shifter.init() or shifter.add(). In init/add either call back if there's a supplied callback or process.exit(1) (in the case of an error) if there is no callback.

unkillbob · 2014-03-12T08:53:58Z

lib/log.js

 exports.err = function (str) {
    if (!silent) {
        console.error(prefix, exports.color('[err]', 'red'), str);
    }
 };

+exports.error = exports.err;


Wasn't sure whether to keep this around or not - I believe I've replaced all uses of it with log.err. Alternatively I could refactor log.err and all calls thereof to be called log.error now that we only need one method?

I think this is ok for now.

unkillbob · 2014-03-12T09:02:17Z

@caridy ready for another round of review :)

unkillbob · 2014-03-19T08:54:35Z

Bump :) Any chance we can get an update on this please?

caridy · 2014-03-19T14:31:31Z

lib/builder.js

-                prebuild(json.postbuilds, options, function () {
+                prebuild(json.postbuilds, options, function (err) {
+                    if (err) {
+                        return buildCallback(err);


do we need to check if buildCallback is set?

build.start is only called from index.js, and both calls pass an explicit callback, it should be safe.

ok, fair enough.

caridy · 2014-03-19T14:47:31Z

lib/module.js

                    }
-                    log.err(name + ': ' + err);


there is something weird here, one of them was calling log.err() the other path was calling log.error(), which means one continues with the execution and the other halt. It seems that the behavior changed.

the return is also weird here.

I think I was under the impression calling back with an error from here would not call all the way back up the stack but after further understanding the stack module I see this isn't the case. I'll change the if/else/if to just do some logging and then always callback(err, result).

Although I did feel like the "file has not changed" error probably shouldn't be considered an error?

As for the previous log.error() vs log.err() here I think the paths that continued execution despite an error were still essentially fatal, they'd just clean up and callback up the stack instead of killing the process on the spot.

caridy · 2014-03-19T14:59:11Z

ok, finished with the second pass. I'm very supportive of this change, and in fact I need this change for locator-yui who relies on shifter. it is shaping up well, so let's get it done.

one thing I want to make clear, we should keep it as simple and as close to the API and behavior as it was before, there is no room for errors here since a bunch of people uses it for building their modules.

so, here is what I will like to see:

less changes in tests (ideally if the test failed before with precess.exit() when called without a callback, it should remain the same.
not changing the behavior of log.err() vs log.error(), in some places I see you're changing that.
clean up of the PR based on some of the feedback (the needed).

unkillbob · 2014-03-19T20:41:00Z

less changes in tests (ideally if the test failed before with precess.exit() when called without a callback, it should remain the same.

The only tests I changed were:

6-builder-uglify-calendar.js - this was relying on the duck-punched process.exit to incorrectly build the module anyway despite that it should have failed the build due to lint errors. Split the testing of those 2 behaviours (fail on lint error, and building the calendar module) out to separate tests.
general.js - the only test I changed the behaviour of was testing the pack module. It doesn't make sense for this small module to process.exit(), it should callback with an error and let the calling code determine how to handle that error. I think it was the right thing to do to refactor the test accordingly.

I'll push another commit shortly with some of the other suggestions you've made.

unkillbob · 2014-03-19T22:07:51Z

@caridy ok I think I've addressed all your feedback, ready for round 3!

caridy · 2014-03-21T15:15:25Z

lib/pack.js

-                        log.error('hitting the brakes! failed to parse ' + file + ', syntax error?');
-                        return;
-                    }
+                    mod = flatten(require(path.join(meta, file)));


any reason for removing the try/catch around the flatten fn?

Because returning from this function won't terminate the outer function or loop - the try/catch was moved inside the traditional for loop below. See my comment on line 79.

caridy · 2014-03-21T15:18:21Z

Ok, this is good, I will go ahead and run a battery of tests with some of the projects that are relying on shifter and see how it goes. If we are good, I should get this merged earlier next week. @unkillbob and @ianstigator thanks for all the help with this.

unkillbob · 2014-04-14T22:43:59Z

Any updates on this? Anything we can do to help this along?

caridy · 2014-04-15T00:26:21Z

this will be released soon along with few others changes.

unkillbob · 2014-04-15T00:37:12Z

Yay thanks!

caridy · 2014-04-15T01:24:46Z

@unkillbob there were few issues with PR #120 and #118, trying to solve those before we release a new version of shifter.

andrewnicols · 2014-04-17T02:36:33Z

FYI, I've commented in #120 why how the issue is causing build failures.

Builds should fail with logging during the failure, not at the end. This reverts back to the behaviour prior to yui#115. Build fails still call the appropriate callback function.

… and stacked errors.

caridy · 2014-06-11T23:06:19Z

[email protected]

caridy reviewed Feb 26, 2014
View reviewed changes

Ian Faigao and others added 4 commits March 12, 2014 12:35

Added callbacks on error handling. Removed the log.error call on the …

ef8a3db

…compressor callback to continue processing.

Added callback handling on log.error() calls. Added unit tests and do…

41e40ca

…cumentation

Code improvements. Added 2 more tests for bad uglifyJS builds on comm…

8f0decc

…and line and rollups. Improved the documentation.

Ensure all fatal errors callback and make all callbacks flow back to …

d24b2c1

…the initial call to shifter.init() or shifter.add(). In init/add either call back if there's a supplied callback or process.exit(1) (in the case of an error) if there is no callback.

unkillbob reviewed Mar 12, 2014
View reviewed changes

caridy reviewed Mar 19, 2014
View reviewed changes

unkillbob added 2 commits March 20, 2014 10:50

Improvements as per pull request feedback.

6f2e135

Simpler examples for testing fatal syntax errors.

bfc36cb

caridy reviewed Mar 21, 2014
View reviewed changes

caridy merged commit bfc36cb into yui:master Apr 15, 2014

unkillbob deleted the feature/callback-error-handling branch April 15, 2014 00:36

caridy mentioned this pull request Apr 16, 2014

Child shifter runs should only use the arguments provided #120

Merged

andrewnicols mentioned this pull request Apr 17, 2014

Build fail issues #122

Open

caridy added a commit to caridy/shifter that referenced this pull request Jun 10, 2014

improving error report after PR yui#115. this adds support for queued…

bfd3551

… and stacked errors.

caridy mentioned this pull request Jun 10, 2014

improving error report after PR #115. this adds support for queued and stacked errors. #131

Merged

caridy added a commit that referenced this pull request Jun 11, 2014

bumping version 0.5.0 for PR #131, #128, #115 and #120

21916ef

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Callback on error instead of process.exit when Shifter is called programmatically #115

Callback on error instead of process.exit when Shifter is called programmatically #115

counterflow commented Feb 18, 2014

caridy commented Feb 21, 2014

unkillbob commented Feb 21, 2014

counterflow commented Feb 26, 2014

caridy Feb 26, 2014

caridy commented Feb 26, 2014

unkillbob commented Mar 4, 2014

caridy commented Mar 10, 2014

caridy commented Mar 11, 2014

unkillbob commented Mar 11, 2014

caridy commented Mar 11, 2014

unkillbob Mar 12, 2014

caridy Mar 19, 2014

unkillbob commented Mar 12, 2014

unkillbob commented Mar 19, 2014

caridy Mar 19, 2014

unkillbob Mar 19, 2014

caridy Mar 19, 2014

caridy Mar 19, 2014

caridy Mar 19, 2014

unkillbob Mar 19, 2014

caridy commented Mar 19, 2014

unkillbob commented Mar 19, 2014

unkillbob commented Mar 19, 2014

caridy Mar 21, 2014

unkillbob Mar 22, 2014

caridy commented Mar 21, 2014

unkillbob commented Apr 14, 2014

caridy commented Apr 15, 2014

unkillbob commented Apr 15, 2014

caridy commented Apr 15, 2014

andrewnicols commented Apr 17, 2014

caridy commented Jun 11, 2014

Callback on error instead of process.exit when Shifter is called programmatically #115

Callback on error instead of process.exit when Shifter is called programmatically #115

Conversation

counterflow commented Feb 18, 2014

caridy commented Feb 21, 2014

unkillbob commented Feb 21, 2014

counterflow commented Feb 26, 2014

Choose a reason for hiding this comment

caridy commented Feb 26, 2014

unkillbob commented Mar 4, 2014

caridy commented Mar 10, 2014

caridy commented Mar 11, 2014

unkillbob commented Mar 11, 2014

caridy commented Mar 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkillbob commented Mar 12, 2014

unkillbob commented Mar 19, 2014

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Mar 19, 2014

unkillbob commented Mar 19, 2014

unkillbob commented Mar 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caridy commented Mar 21, 2014

unkillbob commented Apr 14, 2014

caridy commented Apr 15, 2014

unkillbob commented Apr 15, 2014

caridy commented Apr 15, 2014

andrewnicols commented Apr 17, 2014

caridy commented Jun 11, 2014