Skip to content
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

replace lib/sax.js? #55

Open
brodybits opened this issue Jun 21, 2020 · 5 comments
Open

replace lib/sax.js? #55

brodybits opened this issue Jun 21, 2020 · 5 comments
Labels
help-wanted External contributions welcome needs investigation Information is missing and needs to be researched question Contains open questions that need to be answered strategic
Milestone

Comments

@brodybits
Copy link
Member

The existing lib/sax.js module seems to be needed for XMLReader which is used by dom-parser.js.

From #35 (comment) I am wondering if we could replace sax.js with a faster or better SAX parser?

@karfau
Copy link
Member

karfau commented Jun 21, 2020

For less switching back and forth between issues when reading this I'm replicating the relevant information here:

I digged a bit into the details of testing SAX and existing sax parsers.

It could be worth wile to attempt jindw/xmldom/issues/16 (but there are some isues with it that make it hard).

Which brings me to the (big) question if it's a good idea to maintain both a(nother) SAX parser and all the DOM related things in this project. (I have done the groundwork for comparing different libraries in an efficient manner before, this could be extended/replicated to compare some SAX parsers with our implementation.)

@brodybits
Copy link
Member Author

I think it would be ideal to switch to a better known & tested SAX parser, unless we do find something really special about our own parser.

@karfau
Copy link
Member

karfau commented Jul 1, 2020

From my current understanding of SAX parsers, I think the code in lib/sax.js is doing some things out of it's scope:

xmldom/lib/sax.js

Lines 90 to 93 in cad05a1

var doc = domBuilder.doc;
var text = doc.createTextNode(source.substr(start));
doc.appendChild(text);
domBuilder.currentElement = text;

var doc = domBuilder.doc;
var text = doc.createTextNode(source.substr(start));
doc.appendChild(text);
domBuilder.currentElement = text;

which tightly couples it to the current implementations of DOMHandler in lib/dom-parser.js and DOMImplementation in lib/dom.js.

Another place of coupling is here

domBuilder.locator = ...

which again tightly couples it to the current implementations of DOMHandler in lib/dom-parser.js.

Before switching to another sax parser we would need to fix that (without breaking anything of course).

I think my approach to write tests just for lib/sax.js that can later also be executed against other SAX parsers is still valid I think and will reveal more details.

@hydra1983
Copy link

any progress?

@karfau
Copy link
Member

karfau commented Jul 15, 2021

I have a general idea how to go about it, but only managed to do the first very tiny steps, was blocked by other more important/burning topics and low amount of time invested currently.

If there is somebody that wants to support this topic with contributions, please let me know.
(Without any additional capacity, this topic might still be considered for v1, for which there is no timeline, but it might also be delayed further.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help-wanted External contributions welcome needs investigation Information is missing and needs to be researched question Contains open questions that need to be answered strategic
Projects
Status: Has Priority
Development

No branches or pull requests

3 participants