refactor svg2js to use txml

[TobiasNickel]

Hi,
I refactored the svg2js file. The goal was to use txml instead of the sax parser. The reason for that is, that txml is pure js. There will be no issues about compiling any native modules. It is also much faster than the native module.

Please see: The only files changed are package.json, package-lock.json and the svg2js.js. Even dough there is such a fundamental refactoring, all tests are still passing ! ! !

What do you think of this? Do you think this can help people who are using SVGO, running it on various platform (local + docker) and it can help people who do not have a c compiler installed.

DISCLAIMER: I am the author of txml

[strarsis]

Using txml as XML parser fixes a ton of open svgo issues related to XML parsing.
It also removes the necessity of bindings to the sax parser.
Issues that will be closed by using this new parser:

• should keep unicode space char #1150
• Plugin reusePaths implicitly depends on removeXMLProcInst being enabled #1205 (not related, just bug in plugin)
• Double encoding with datauri #997
• Error in parsing SVG: Unexpected end #879
• Support unquoted attributes #678

Edit: Added a PR with tests that pass when tXML parser is used: https://github.com/strarsis/svgo/tree/parser-tests

[strarsis]

@GreLI: It would be great if this can be reviewed and merged! 🐱

[TobiasNickel]

Nice to see there is some new work going on here with svgo. I see the svg parser is now a sync function and I think this is good.

I would refactor this PR to also work synchronous. That would actually be the default behavior of txml.

@TrySound but first I would like to hear your opinion.

[TrySound]

Changing parser is a big shift. I will try to fix existing issues first and then come back to this change.
Do you have a plan on maintaining txml? Any popular project use it already?

Through2 look not necessary. Can be replaced with const { PassThrough } = require('through2');

The reason for that is, that txml is pure js. There will be no issues about compiling any native modules. It is also much faster than the native module.

AFAIK rollup will still hoist lazy requires to convert into es modules. You better split stream based extension into separate entry point.

[TobiasNickel]

hi, I made an update to txml, to work better with rollup. having two entry points as you suggested, that is pretty good. (now on github only, will publish that update soon to npm.) The parser is now 17kb(uncompressed and still include all comments and formatting.)

I tried to run the refactoring I made in this PR, with the new version. All your new updates and updated tests. Local I have it so far, as that all tests run through, but many produce a somewhat different result (not good).

Are you still interested to consider using txml for svgo? Then I would see if I can pass all tests again within the new version. @TrySound @strarsis

[TobiasNickel]

just a little update, on my current work, I have the current and my refactoring in theory produce the same output just compared via JSON.stringify, but there are still errors. Next I will analyze any other differences.

[TobiasNickel]

Wow, it works. all tests passed today.
now I only need to publish the new version of txml, then the regression tests will also pass here.

@strarsis @TrySound how do you like it?

[strarsis]

@TobiasNickel @SethFalco: It would be great if this could be merged! A purely JS, fast and efficiently designed XML parser as drop-in replacement would be a great improvement for svgo.

• Fixes a lot of parsing-specific issues.
• Gets rid of bindings with SAX parser (allows native browser support?).
• Fast and well-maintained library.

[SethFalco]

Hey hey! I'll look at this in future, but for now, I've been prioritizing bug fixes and optimizations in existing plugins, similarly to TrySound when he was reviewing this.

I've also been putting off touching dependencies in general:

• I don't want to negatively impact the bundle/install size without more research. txml (249kB) is substantially larger than sax (53.7kB)
• This project has numerous maintainers, all of which probably have more experience/context than me regarding SVGO. It'd be wrong for me to take action on my own anyway.
• For a project with this much usage, the decision to add/change dependencies is more sensitive in general, imo.

I wanted to look at our XML library anyway, since our "maintained fork" of sax is about as maintained as sax. We may want to consider upstreaming our changes and moving back at least, as the maintainer of sax is alive and even pushed an update lately, though updates are still sparse. ^-^'

If we were to change the library, fast-xml-parser stands out most to me, but we can do an in-depth review when it's actually time to look at this.

Once I've gotten the repository to a more manageable/stable state, it'll be a lot easier to pester all the maintainers to spare some time to discuss this publicly in an RFC (GitHub issue). I'll ping you in it too ofc.