Leonidas-from-XIV / node-xml2js

XML to JavaScript object converter.
MIT License
4.91k stars 606 forks source link

Why do we need a callback for parseString()? #380

Open zszszsz opened 7 years ago

zszszsz commented 7 years ago

Since the function parseString is not asynchronous at all ?

(() => {
    var a;
    new require('xml2js').Parser().parseString('<xml>asdf</xml>', (e, r) => { a = r });
    console.log(a);
})();

and the output is:

{ xml: 'asdf' }
Leonidas-from-XIV commented 7 years ago

Backward compatibility, since changing the signature breaks all existing depending libraries.

DoomTay commented 7 years ago

Didn't stop the guy behind jsdom. He recently rewrote his API to be callback-less (though still provides a way to use the old API, at least temporarily.)

jfsiii commented 7 years ago

@Leonidas-from-XIV how do you feel about adding parseStringSync which returns the result?

jeghers commented 7 years ago

I am trying to use this in a Redux saga, but the callback structure is a showstopper since "yield" won't work inside the callback.

PhilipDavis commented 6 years ago

Yeah, it's a hassle to not have a synchronous version of parseString. Thanks @zszszsz for a concise workaround.

Anyways, @jeghers See https://redux-saga.js.org/docs/api/#cpsfn-args for using the node-style callback in Redux-Saga

e.g. something like:

const js = yield cps(parseString, xml);
jeghers commented 6 years ago

Cool thanks!

On Jan 4, 2018 12:17 PM, "Philip Davis" notifications@github.com wrote:

Yeah, it's a hassle to not have a synchronous version of parseString. Thanks @zszszsz https://github.com/zszszsz for a concise workaround.

Anyways, @jeghers https://github.com/jeghers See https://redux-saga.js.org/docs/api/#cpsfn-args for using the node-style callback in Redux-Saga

e.g. something like:

const js = yield cps(parseString, xml);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Leonidas-from-XIV/node-xml2js/issues/380#issuecomment-355387541, or mute the thread https://github.com/notifications/unsubscribe-auth/AGaRneIG0ue3IAuVE-WPVVXHw5kK0eltks5tHTHUgaJpZM4NLbyR .

zszszsz commented 6 years ago

@PhilipDavis This parseString is itself synchronous in nature, it is not a workaround actually.

@jeghers you can write you own parseStringSync using the techniques or "workaround" above, exp.

function parseStringSync (str) {
    var result;
    new require('xml2js').Parser().parseString(str, (e, r) => { result = r });
    return result;
}

, and you can avoid using yield in callback

jeghers commented 6 years ago

That looks good, thanks for the input!

On Jan 4, 2018 12:26 PM, "zszszsz" notifications@github.com wrote:

@PhilipDavis https://github.com/philipdavis This parseString is itself synchronous in nature, it is not a workaround actually.

@jeghers https://github.com/jeghers you can write you own parseStringSync using the techniques or "workaround" above, exp.

function parseStringSync (str) { var result; new require('xml2js').Parser().parseString(str, (e, r) => { result = r }); return result; }

, and you can avoid using yield in callback

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Leonidas-from-XIV/node-xml2js/issues/380#issuecomment-355389602, or mute the thread https://github.com/notifications/unsubscribe-auth/AGaRnaY2nxjd_muG6doVZcQXZltXHKFDks5tHTQBgaJpZM4NLbyR .

PhilipDavis commented 6 years ago

@zszszsz parseString doesn't return the result directly. Hence, your workaround to retrieve it using a closure. What do you call it if not a workaround?

Leonidas-from-XIV commented 6 years ago

@jfsiii The parser library that we use, sax.js also provides an async API AFAIK, so unless they provide one there is no clean way to implement parseStringSync in a non-hacky way (like the closure workaround posted here).

jcsahnwaldt commented 6 years ago

I added this function to my copy of parser.coffee. Seems to work quite nicely, and I don't think it's hacky. Should I submit a pull request?

parseStringSync: (str) =>
  res = undefined
  err = undefined
  @on "end", (r) ->
    @reset()
    res = r
  @on "error", (e) ->
    @reset()
    err = e

  @saxParser.write(str).close()

  if err?
    throw err
  else
    return res

I haven't tested it thoroughly yet. The error handling in the original parseString is much more elaborate. I omitted it because I didn't understand it. parseStringSync seems to work fine as it is. When the SAX parser throws an exception, parseStringSync simply lets that exception pass through to the caller, which is fine.

I also omitted the handling of BOMs and empty strings for my tests, but I can easily put that back in.

zhaoxiongfei commented 5 years ago

The process of parsing xml is CPU exclusive type, So async meaningless. Direct return is a better solution.

scripting commented 3 years ago

I'm want to build a package on top of xml2js to parse OPML specifically, to make it super easy for people to use. I want to emulate the way JSON.parse and JSON.stringify work. Do I really need to have a callback?

Two possible solutions:

  1. Couldn't the parse routine "just work" if the callback is undefined?
  2. Have another entry-point to the package called parseSync that returns the JavaScript structure for the XML and throws an error if there's an error.
scripting commented 3 years ago

This is how xml2js.parseStringSync would work.

function parse (xmlltext) {
    var options = {
        explicitArray: false
        };
    return (xml2js.parseStringSync (xmlltext, options));
    }
suchakraborty commented 2 years ago

I wonder, collectively, how many hours people have wasted stumbling upon this.... During an integration/refactoring, converting json to xml (dont ask), I almost made rewrote everything into promises/async because I assumed this is async. So tally up +1 hr and a 5 minutes debate from my end. 😄 One time rookie mistake - but expensive.