Use first argument of callback instead of throw on error

[Rundfunk]

Hi Leonidas,

While working with xml2js, I found that the first argument of the callback is not used when an error is encountered (like parsing incorrect XML), instead throw is used. (See line 183 of xml2js.coffee.)

Why is an error thrown? This requires the usage of a try/catch block, instead of checking if err is set.

Let's say I have the following code where I check the first argument of the callback:

parseString xml, (err, result) ->
    console.log 'encountered error' if err

This does not work at the moment, causing the need for a try/catch block:

try {
    parseString xml, (err, result) ->
        # process result
} catch (e) {
    console.log 'encountered error', e
}

However, when the # process result becomes very long, the code for which the try is needed and the catch become distanced from each other, making it difficult to read.

To summarize my question: can the first argument of the callback be used to communicate an error instead of throwing one?

[Leonidas-from-XIV]

Hi Rundfunk,

You are completely right, looks like this was introduced in a644bfd - before we used to have exactly the behaviour that you wanted. I just reverted this and released xml2js 0.2.4 as a bugfix release.

[Rundfunk]

Hi Leonidas,

Thank you for your swift reply and corresponding commit. I have found that in some cases the fix does work. For example in this case:

parseString = require('xml2js').parseString;

buggyXml = '<?xml version="1.0"?><a><b>x</a></b>';

parseString(buggyXml, function (err, result) {
    if (err) {
        console.log('An error occurred: ', err);
    } else {
        console.log('All is fine!');
    }
});

The program will state An error occurred:, followed by an explaination of the error. (Unexpected close tag.)

However, this is not the case with all XML files. In the following example, an error is emitted from line 180 of xml2js.coffee (@emit("error", ex.message)):

parseString = require('xml2js').parseString;

buggyXml = '<?xml version="1.0"?><a><b>x</b></a><b>x</b>';

parseString(buggyXml, function (err, result) {
    if (err) {
        console.log('An error occurred: ', err);
    } else {
        console.log('All is fine!');
    }
});

In this case, I first get the message All is fine!, followed by the error Error: Uncaught, unspecified 'error' event..

As far as I understand, this is because the error in the second program is handled according to the "Traditional" usage, as it is called on the homepage. However, it would be good if all errors could be checked with an if (err), because now some errors are passed on to the callback as the first argument, while others are emitted.

[Leonidas-from-XIV]

Oh, I'm sorry. But thanks a lot for the examples, I'll add them to the testsuite and will try to fix it, so that it does not revert back.

[Leonidas-from-XIV]

Well, this is strange, because with the current master revision I get err as null because my sax version was apparently able to parse some nonsense out of your deliberately incorrect examples:

// first example
{ a: { _: 'a>', b: [ 'x' ] } }
// second example
{ a: { b: [ 'x' ] } }

So it is hard to test for errors, because the sax parser does not return any.

Over some time, I'd like to get rid of the "traditional usage" as it turns out it is a mess and nobody likes it anyway (see issue #69).

[Leonidas-from-XIV]

@Rundfunk Can you submit an unit test that reproduces this behaviour? Greatly appreciated!

[Rundfunk]

Hi @Leonidas-from-XIV; what I can currently offer you are the two examples I posted above, as my skills with unit testing in Node.js are still limited.. Still hope the examples help.

On the "traditional usage": things may become easier to maintain and use when there is only one way of interacting with the library. So my opinion would also be to only keep the "new" usage style.

[Leonidas-from-XIV]

That's strange because for me they don't cause errors (and yes, they should but the parser doesn't really complain). Have you tried the sax.js-version that I recently pinned?

On the "traditional usage": that one will probably go away in 0.3.

[jcsahnwaldt]

Looks like this issue still occurs in xml2js 0.4.19.

const xml2js = require('xml2js');
let xml = `<a><b>x</a></b>`;
try {
  xml2js.parseString(xml, (err, res) => {
    console.log('callback err:');
    console.log(err);
    console.log('callback res:');
    console.log(res);
  });
}
catch (err) {
  console.log('caught err:');
  console.log(err);
}

Output (paths in stacktrace sanitized):

callback err:
Error: Unexpected close tag
Line: 0
Column: 11
Char: >
    at error (./node_modules/sax/lib/sax.js:651:10)
    at strictFail (./node_modules/sax/lib/sax.js:677:7)
    at closeTag (./node_modules/sax/lib/sax.js:871:9)
    at SAXParser.write (./node_modules/sax/lib/sax.js:1436:13)
    at Parser.exports.Parser.Parser.parseString (./node_modules/xml2js/lib/parser.js:322:31)
    at Parser.parseString (./node_modules/xml2js/lib/parser.js:5:59)
    at Object.exports.parseString (./node_modules/xml2js/lib/parser.js:354:19)
    at Object.<anonymous> (./test.js:6:10)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
callback res:
undefined
caught err:
Error: Unmatched closing tag: b
Line: 0
Column: 15
Char: >
    at error (./node_modules/sax/lib/sax.js:651:10)
    at strictFail (./node_modules/sax/lib/sax.js:677:7)
    at closeTag (./node_modules/sax/lib/sax.js:879:7)
    at SAXParser.write (./node_modules/sax/lib/sax.js:1436:13)
    at Parser.exports.Parser.Parser.parseString (./node_modules/xml2js/lib/parser.js:322:31)
    at Parser.parseString (./node_modules/xml2js/lib/parser.js:5:59)
    at Object.exports.parseString (./node_modules/xml2js/lib/parser.js:354:19)
    at Object.<anonymous> (./test.js:6:10)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)

First, the callback function is called with an err parameter, but then an exception is thrown with a slightly different err object.