push frames without context on file read errors in the parser

[dan-manges]

I think the intention on file read errors is to push the frame without context. There's a comment in the parser that says:

// There was an error reading the file, just push the frame and
// move on.

However, because looper was being called instead of pendingCallback, an error reading a file is causing rollbar to bubble the exception up through all the callbacks and never notify.

[dan-manges]

any thoughts on this @brianr or @sbezboro?

[brianr]

Hey @dan-manges , sorry for the delay, will try to find some time to review and merge today.

[brianr]

Is there any particular reason you did this instead of:

frames.push(frame);
return;

[brianr]

Er,

frames.push(frame);
return looper(null);

I think I'm going to merge as-is for now, but wanted to mention.

[brianr]

Merged, thanks! This will get pushed to npm in 0.3.3, which will be later today.

[dan-manges]

Thanks for merging this @brianr. I think the only reason I called pendingCallback instead of pushing the frame and calling looper was because of the comment near the top of pendingCallback:

// There was an error reading the file, just push the frame and
// move on.
if (err) {
            frames.push(frame);

I thought that maybe the intent was to have all error handling flow into that logic branch but somebody accidentally called looper instead of pendingCallback. But that might not have been the intent, and I like your suggestion of pushing the frame immediately. Let me know if you want me to change it.

[brianr]

@coryvirok do you have an opinion here?

[coryvirok]

@dan-manges, thanks for the fix. @brianr, I prefer the way @dan-manges has it here so we only handle the error case in 1 place.

This code is way too complicated. I'll file a task to refactor.