Add promise-specific version of parseString

[mckramer]

A possible solution for #421. Due to the "async" option, it did not seem possible to convert the existing implementation, so, instead, use util.promisify (via shim to support node < 8).

Examples:

// Via root API
xml2js.parseStringPromise('< ... >', options)
  .then(function (result) { /* ... */ })
  .catch(function (err) { /* ... */ })

// Via parser
var parser = new xml2js.Parser(options);
parser.parseStringPromise('< ... >')
  .then(function (result) { /* ... */ })
  .catch(function (err) { /* ... */ })

[Leonidas-from-XIV]

It fails on all CI environments :-(

[coveralls]

Coverage increased (+0.07%) to 97.701% when pulling 6353f53 on mckramer:parse-string-promise into e8540dc on Leonidas-from-XIV:master.

[mckramer]

@Leonidas-from-XIV, I did not realize I needed to commit the compiled JS as well. I did that now. This PR fails on node 0.10 + 0.12. As those are EOL, do you want to keep supporting them? I also opened #430 in case you wanted to consider dropping support for EOL versions.

[Leonidas-from-XIV]

@mckramer Yeah, the files are there to lower the amount of dependencies required to use the project. This was before compiling was super-popular in JS. For now they are there, when I re-do things I'll think of some better solution since no-one likes generated files in the repository anyway 😄

If you rebase your branch onto master you should get more recent version of Node 👍

[Leonidas-from-XIV]

I just thought of something: maybe it is ok if parseString returned a promise if no callback was passed? That way there is no additional function. The downside is that parseString will have very confusing behaviour.

[j3k0]

maybe it is ok if parseString returned a promise if no callback was passed

Maybe it could even always return a Promise, regardless of the callback argument. So far, parseString doesn't have a defined returned value.

[Leonidas-from-XIV]

Yes, this is *way* better, also makes the typing less surprising.

[mckramer]

Sorry for delay in coming back to this.

I was not able to find a way to make the promise returnable always without breaking the existing API in some way. @j3k0 mentioned it in #441, but there are unit tests that rely on sync exceptions and returns (and some odd behavior with the "async" option return). Maybe you can consider those unit tests not an intended API, but it is technically breaking, which is why I opted to create a separate parseStringPromise method.

Either way would be fine with me, but I do think that if you go always return promise (i.e. #441), you should bump the "major" version and note the change, in case anyone is relying on the existing behavior.

[miguel-orange]

@mckramer - Thanks for putting this PR together.

[rafaelklaessen]

I'm not trying to hurry you guys, but is this PR going to be merged?

[buphmin]

If anyone is interested in the meantime here is a typescript example of how you can write your own wrapper with @types/xml2js.

import {convertableToString, OptionsV2, parseString} from 'xml2js';

function parseStringAsync(xml: convertableToString, options?: OptionsV2): Promise<any> {
  return new Promise((resolve, reject) => {
    if(!options) {
      parseString(xml, (err, results) => {
        if(err) {
          reject(err);
        }

        resolve(results);
      });
    } else {
      parseString(xml, options, (err, results) => {
        if(err) {
          reject(err);
        }

        resolve(results);
      });
    }
  })
}

Edit: Swapped the logic around on the wrapper for future viewers :)

[alexkb]

@buphmin thanks for sharing your snippet. You may want to swap the if statement blocks around though, as currently it won't pass the options parameter to parseString().

[bradleat]

+1

[yunfan]

If anyone is interested in the meantime here is a typescript example of how you can write your own wrapper with @types/xml2js.

import {convertableToString, OptionsV2, parseString} from 'xml2js';

function parseStringAsync(xml: convertableToString, options?: OptionsV2): Promise<any> {
  return new Promise((resolve, reject) => {
    if(options) {
      parseString(xml, (err, results) => {
        if(err) {
          reject(err);
        }

        resolve(results);
      });
    } else {
      parseString(xml, options, (err, results) => {
        if(err) {
          reject(err);
        }

        resolve(results);
      });
    }
  })
}

thanks for the helpful fix, but there is a Logical reverse in the code.

[knoxcard]

@mckramer - nice job!

[Leonidas-from-XIV]

It didn't close the PR automatically but rest assured I just merged @mckramer's changes into master.