-
Notifications
You must be signed in to change notification settings - Fork 607
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Why do we need a callback for parseString()? #380
Comments
Backward compatibility, since changing the signature breaks all existing depending libraries. |
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.) |
@Leonidas-from-XIV how do you feel about adding |
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. |
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:
|
Cool thanks!
…On Jan 4, 2018 12:17 PM, "Philip Davis" ***@***.***> 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
<#380 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGaRneIG0ue3IAuVE-WPVVXHw5kK0eltks5tHTHUgaJpZM4NLbyR>
.
|
@PhilipDavis This @jeghers you can write you own
, and you can avoid using |
That looks good, thanks for the input!
On Jan 4, 2018 12:26 PM, "zszszsz" <[email protected]> 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
<#380 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGaRnaY2nxjd_muG6doVZcQXZltXHKFDks5tHTQBgaJpZM4NLbyR>
.
|
@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? |
@jfsiii The parser library that we use, |
I added this function to my copy of 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 I also omitted the handling of BOMs and empty strings for my tests, but I can easily put that back in. |
The process of parsing xml is CPU exclusive type, So async meaningless. Direct return is a better solution. |
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 Two possible solutions:
|
This is how xml2js.parseStringSync would work.
|
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. |
Since the function parseString is not asynchronous at all ?
and the output is:
The text was updated successfully, but these errors were encountered: