-
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
Sync version of parseString #159
Comments
What exactly do you mean by "sync"? xml2js is (unfortunately) currently sync because the AWS bindings depend on being sync. |
Sorry for not being clear. parseString(xml, function (err, result) {
console.dir(result);
}); but instead I want to be able to run var result = parseString(xml);
console.dir(result); Is it possible in this or any other way? |
No, that would be explicitly blocking behaviour. That is what I want to get away from. You can interact with xml2js using any Async library like async-q or Promise.js. |
One area where this becomes a problem is karma.conf.js files, which must be synchronous (more info). I work on a java project that specifies it's css/js dependencies in an xml file. It'd be nice use to use xml2js to populate the config.files property, rather than maintain two sets of dependency lists. Agreed that blocking behavior is usually something to avoid, but this is an area where a synchronous version of parseString() would come in handy. |
Sorry, but that's really not possible. sax.js does not support a synchronous mode of operation that I know of, so I do have to pass it some callbacks to do the parsing. I'd be gladly proven wrong in a PR, though. |
This PR is over-kill. As Leonidas said just use promise to wrap like this: async function xml2json(xml) {
return new Promise((resolve, reject) => {
parseString(xml, function (err, json) {
if (err)
reject(err);
else
resolve(json);
});
});
} |
Async ain't all it's cracked it up to be. Promises doesn't make it async either. Call/Await isn't available in node.js programming. Why should this call be async anyway, I'm passing it a string, where's the bound.io you are trying to prevent, async is costing time here, not saving it. I don't know why authors of these libraries need to hand cuff us because they think one way is better than another, it's easier to writing a sync version, why not just have both? Constant context switching between threads for such a simple task will cost more time than it will ever save ... |
@HoosierMike It is async because that's the interface that the underlying parser exposes. |
If the underlying api is async, how comes this works fine:
Is the underlying API just pretending to be async? Because |
Thanks for the mocking tone, I really appreciate it.
For async operation you can try the `async` option. The reason why it takes a callback yet operates in a sync fashion is because it was this way when I inherited the library. When I tried to make this actually async, it turned out it broke Amazons libraries which depend on the fact that the callbacks are actually still synchronous. That's a default that should probably be changed in the future, but in fact there are lots of things that could be made better but for compatibility reasons and time reasons, aren't.
|
I didn't intend to mock you, but the api communicates that it actually async, while it apparently is not. Callback != async. People are asking for a sync version of it, and it is right there. You mention that you want to get away from blocking behavior, but that just makes no sense. There is no IO going on that would motivate it being async. I suppose the underlying implementation could start it's own process, but that would be extreme overkill for small snippets of xml. If performance is an issue, the api should probably be stream-based anyway. |
The API communicates a callback which is required due to how the underlying SAX parser works, there's not much I can do about it apart from taping a function around which "uncallbacks" it, but that seems unclean to me as well as it implies that SAX.js will always be sync and breaks horribly otherwise. Blocking behaviour does not have to be IO, runtimes like Erlang also allow preempting on computation which in the case of parsing huge XML files would be nice but I don't see that happening soon. Streaming APIs are nice but not that great of a match for xml2js, since it is built to output a single parsed object, if streaming is required users can probably just read sub-blocks from their input and send them through xml2js individually. |
Yeah, this is really sad. We all use |
Apologies for not replying sooner. I originally wrote a pull request that adds a "parseSync" method to the API, but in retrospect I don't fault @Leonidas-from-XIV for not including it, and I think that the rationale for not changing the API is perfectly valid. Also, as @geon points out, it's possible to write a sync-style wrapper function. This is generally not considered a smart move (if the underlying library becomes non-blocking someday you're toast), but I imagine it yields better results than sending passive-aggressive notes to an unpaid developer. |
Actually, you are not toast. You still free to continue using the old version. And as you can see, the code I posted will detect if a newer version is async. |
Yeah, I meant "toast" in the respect that the sync method would no longer be synchronous. Thanks though. |
@Leonidas-from-XIV Sorry to reopen this unfortunately charged issue, but would you be willing to take #241 if it was cleaned up and comments addressed? The callback API you have here is kind of weird, because it explodes with an unhandled exception instead of passing the error to the callback I explicitly provide. I was using this with Rx and it took me a while to understand what was going on. |
@davidhq thanks |
For anyone looking for a non-async alternative try
|
Within an const json = await new Promise((resolve, reject) => {
parseString(xml, (err, result) => err ? reject(err) : resolve(result) );
}); |
Correct, this library takes a string already presumed to be loaded in memory and returns a transformed string. It is a synchronous operation by nature. The " What could be considered |
@hcapp01 - if you use
|
Wrapping
|
How can I run sync version of parseString() function?
The text was updated successfully, but these errors were encountered: