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

chore: use external xast type declarations #1805

Closed
wants to merge 1 commit into from

Conversation

SethFalco
Copy link
Member

@SethFalco SethFalco commented Sep 30, 2023

Removes our custom type declarations for Xast in favor of the ones suggested by xast.

If you are using TypeScript, you can use the xast types by installing them with npm:

npm install @types/xast

https://github.com/syntax-tree/xast#types

Motivation

While it makes sense to declare types of Xast for our plugin API, I think it'd be better to avoid maintaining them as part of this library/application if there are conventional solutions available already.

  • Imo it's better to leave maintaining/redistributing them to @DefinitelyTyped, which Titus Wormer and Christian Murphy from syntax/xast helped define.
  • The new types more strictly adhere to the Xast spec, which should simplify integrating with Xast libraries in future.

My hope was actually to explore some libraries surrounding Xast, in particular, xast-util-from-xml and xast-util-select. Unfortunately, neither of these meet fulfil our needs atm.


Not sure if this is a worthwhile change, so feedback welcome.

@TrySound
Copy link
Member

Working with attributes with these types is less trivial though parser produce only string values. I'd prefer to stay with old types.

This change also introduce 2 more packages. xast-util-from-xml would bring even more.
Last couple months I had very bad Internet connection and loading myriads of packages in node_modules made it worse. So I'd encourage you to keep svgo more concentrated.

Underlying xml parser seem interesting though. We could write own wrapper around it and replace sax.

@SethFalco
Copy link
Member Author

SethFalco commented Sep 30, 2023

Sounds good, I'll shelve this then!

Underlying xml parser seem interesting though.

Unfortunately, one thing that will stop us from using it for now is the lack of support for DTD entities.

https://github.com/rgrove/parse-xml#not-features

Not sure if that's planned, or something we'd be able to work around. I'll review both later, but right now, I'm more focused on documentation.

WIP:



@SethFalco SethFalco closed this Sep 30, 2023
@TrySound
Copy link
Member

Awesome!

@TrySound
Copy link
Member

TrySound commented Oct 3, 2023

Wrote to you in matrix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants