- Notifications
You must be signed in to change notification settings - Fork58
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Callback on error instead of process.exit when Shifter is called programmatically#115
Conversation
@ianstigator I will look into this PR soon, but for the note about |
@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). |
@caridy All tasks have been completed. No pending actions to do. This is now ready for a review. |
| @@ -57,11 +57,15 @@ exports.warn = function (str) { | |||
| } | |||
| }; | |||
| exports.error = function (str) { | |||
| exports.error = function (str, callback) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
I want to encourage you to rethink the error callback logic since |
@caridy thanks for the feedback, done some thought on this and I propose:
Does that sound like a cleaner approach (rather than passing the callback to |
hey@unkillbob, @ianstigator, I haven't forgot about this, will get back to it on monday. |
@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. |
@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. |
Ok@unkillbob, I was able to reproduce that. Alright, let's get this PR clean up. |
…compressor callback to continue processing.
…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.
| exports.err = function (str) { | ||
| if (!silent) { | ||
| console.error(prefix, exports.color('[err]', 'red'), str); | ||
| } | ||
| }; | ||
| exports.error = exports.err; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wasn't sure whether to keep this around or not - I believe I've replaced all uses of it withlog.err. Alternatively I could refactorlog.err and all calls thereof to be calledlog.error now that we only need one method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this is ok for now.
@caridy ready for another round of review :) |
Bump :) Any chance we can get an update on this please? |
| prebuild(json.postbuilds, options, function () { | ||
| prebuild(json.postbuilds, options, function (err) { | ||
| if (err) { | ||
| return buildCallback(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do we need to check ifbuildCallback is set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
build.start is only called from index.js, and both calls pass an explicit callback, it should be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, fair enough.
| } | ||
| log.err(name + ': ' + err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is something weird here, one of them was callinglog.err() the other path was callinglog.error(), which means one continues with the execution and the other halt. It seems that the behavior changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the return is also weird here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 thestack module I see this isn't the case. I'll change the if/else/if to just do some logging and then alwayscallback(err, result).
Although I did feel like the "file has not changed" error probably shouldn't be considered an error?
As for the previouslog.error() vslog.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.
ok, finished with the second pass. I'm very supportive of this change, and in fact I need this change for 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:
|
The only tests I changed were:
I'll push another commit shortly with some of the other suggestions you've made. |
@caridy ok I think I've addressed all your feedback, ready for round 3! |
| log.error('hitting the brakes! failed to parse ' + file + ', syntax error?'); | ||
| return; | ||
| } | ||
| mod = flatten(require(path.join(meta, file))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
any reason for removing the try/catch around the flatten fn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because returning from this function won't terminate the outer function or loop - the try/catch was moved inside the traditional for loop below. Seemy comment on line 79.
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. |
Any updates on this? Anything we can do to help this along? |
this will be released soon along with few others changes. |
Yay thanks! |
@unkillbob there were few issues with PR#120 and#118, trying to solve those before we release a new version of shifter. |
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. Thisreverts back to the behaviour prior toyui#115.Build fails still call the appropriate callback function.
Builds should fail with logging during the failure, not at the end. Thisreverts back to the behaviour prior toyui#115.Build fails still call the appropriate callback function.
… and stacked errors.
|
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.