Make sax_ltx recognize CDATA

[swissmanu]

as suggested by issue #19, this PR introduces correct CDATA parsing for sax_ltx. unit test is available.
what do you think about the implementation? i dont like the way how i had to implement the look ahead for detecting CDATA at all. any other ideas?

[sonnyp]

@swissmanu what your use case? IIRC CDATA isn't XMPP valid

[astro]

ltx isn't limited to XMPP

[sonnyp]

@astro true but if there is very low demand on ltx for non-XMPP usage I'd be very careful about adding this. ltx is grrrreat but there might be better alternative out there for non-XMPP. (maybe not, no idea)

[astro]

I've always reached out to ltx for any XML parsing needs. The API feels incredibly intuitive, having designed it myself. :-P

[swissmanu]

Thank you for your responses 👍

I got into this problem when tinker with the XMPP server of Logitechs Harmony hub (a small device that emits IR and radio commands to your entertainment system)
As long as node-xmpp was using the native-bound node-expat, I had no problem to parse the responses emitted by the server. When switching to ltx, the CDATA-containing responses became unparsable.

My intention was to extend ltx so it becomes more XML-compliant. If its only thougth to be used in an XMPP environment, then it might be the wrong decision to extend it in that direction :)

[astro]

So, there are XMPP implementations that use CDATA? I vote for ignoring all specifications and implementing this.

[lloydwatkin]

@astro I don't think that is what @swissmanu is suggesting. He's saying that if ltx was designed for XMPP only then he agrees that CDATA shouldn't be supported, but if not (as is the case) then CDATA should be supported.

[sonnyp]

Maybe we should have sax_ltx (with CDATA; XML 1.1; ...) and sax_xmpp. Then make node-xmpp projects use the second.

[swissmanu]

@sonnyp that sounds like a good compromise as long as i'm allowed to interchange them as a like since i have the situation where a an xmpp server emits a CDATA.

[lloydwatkin]

Personally I don't think mixing xmpp specific stuff into ltx is worthwhile, it should just be a generic xml parser/builder. If people have cdata in their stanzas then that's up to them :)

[sonnyp]

@lloydwatkin parsing XML 1.1 and CDATA has a performance impact and in 99% cases you don't need those for XMPP where it's not standard.

@swissmanu you could use the sax_saxjs.js parser (you'll probably need to map more events)
sax-js has support for cdata https://github.com/isaacs/sax-js#events

Overall I don't really see the point in re-writting a XML 1.1 compliant sax JS parser as sax-js seems to do the job pretty well.
I'd rename sax_ltx sax_xmpp; make sax_saxjs the default and use sax_xmpp as default in node-xmpp-core/server/client

[lloydwatkin]

Ahh sorry, wasn't aware of the performance impact!
On 21 May 2015 09:10, "Sonny Piers" notifications@github.com wrote:

@lloydwatkin https://github.com/lloydwatkin parsing XML 1.1 and CDATA
has a performance impact and in 99% cases you don't need those for XMPP
where it's not standard.

@swissmanu https://github.com/swissmanu you could use the sax_saxjs.js
parser (you'll probably need to map more events)
sax-js has support for cdata https://github.com/isaacs/sax-js#events

Overall I don't really see the point in re-writting a sax JS parser as
sax-js seems to do the job pretty well.
I'd rename sax_ltx sax_xmpp; make sax_saxjs the default and use sax_xmpp
as default in node-xmpp-core/server/client

—
Reply to this email directly or view it on GitHub
#51 (comment).

[sonnyp]

closing this due to inactivity.

@swissmanu if you still have interest in this I'll accept the PR if you copy current sax_ltx to sax_xmpp or something like that

[swissmanu]

aaaah totally missed your reply @sonnyp. i'll revise my change and integrate it into sax_xmpp. thx!

[sonnyp]

I was wrong, cdata is XMPP compliant (and probably a must have for node-xmpp).
cdata is part of xml 1.0 http://www.w3.org/TR/2008/REC-xml-20081126/#sec-cdata-sect
https://xmpp.org/rfcs/rfc6120.html#xml-restrictions does not explicitly restrict cdata and
https://xmpp.org/rfcs/rfc3923.html makes use of them

@swissmanu no need to create a new parser, just rebase your PR / fix tests

[sonnyp]

github doesn't allow me to re-open the PR, @swissmanu I believe you'll have to create a new one

[sonnyp]

@swissmanu

what do you think about the implementation? i dont like the way how i had to implement the look ahead for detecting CDATA at all. any other ideas?

I get the feeling but I like how it doesn't interfere too much with the rest of the code. 👍 and maybe we can come up with a more 'elegant' solution to this in the future.

[sonnyp]

@swissmanu your PR was rebased by @timdp

Released as 2.7.0, thanks to both of you