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

svgo improvements #777

Closed
caub opened this issue Aug 16, 2017 · 22 comments
Closed

svgo improvements #777

caub opened this issue Aug 16, 2017 · 22 comments

Comments

@caub
Copy link
Contributor

caub commented Aug 16, 2017

I wanted to make svgo more flexible, using jsdom, but that change a lot plugins usage, so I started https://github.com/caub/svgz, I'd like to port a bunch of svgo plugins there.

So far it's going good, I still have some big plugins to convert like the ones with path data, but the rest is often easier to do

I'd be glad to set svgo main contributors owners there, and have contributors, if people like this direction

@GreLI
Copy link
Member

GreLI commented Aug 16, 2017

🤔

@caub
Copy link
Contributor Author

caub commented Aug 16, 2017

maybe @deepsweet would see the idea interesting? not trying to steal anything, just trying to make things easier to write plugins (using the bare DOM api, which is now quite nice to use (new additions like .after, .append, .closest, ...) and familiar to anyone) (one example: some plugins need to know if there are any <script> or <style>, it becomes easier to do, and more correct (because I think it's often detected during the tree walk, but it should be done beforehand). I'm not sure if it'll be slower/faster, that's what I was trying to see

@deepsweet
Copy link
Member

My intent (happened few times actually) to rewrite SVGO from a scratch was based exactly on JSDOM. I see many benefits:

  • DOM Tree for free
  • DOM API for free
  • It should be possible to abstract SVGO core from Nodejs at all so we can have a build for browser (I remember there were 1+ requests for this)

I had some very raw prototype but now it's just lost in time.

Another ideas which I'd like to see if someone would like to take over this huge effort:

  • ESNext all the things with Babel, Nodejs 4+
  • modularize
    • monorepo
    • each plugin svgo-plugin-foo is a package
    • decouple svgo-cli, "API first"
    • presets
      • svgo-preset-safe
      • svgo-preset-aggressive
  • Cheerio on top of JSDOM if it's really necessary for even more handy DOM tree traverse/mutation/etc
  • Strict semver with no fear of positive integers and much faster release cycle

Regarding "svgz" in particular – I don't personally think that it's a good idea to have it as a separate project and split the effort. Many SVG optimizations are based on pretty complex math and geometry, I know zero to few JS devs who are good at it and able to fix such an issues. @GreLI did a really amazing job on maintaining SVGO and he knows about this a lot, more than me actually :)

My point is to 1) discuss more tech details 2) create a branch 3) collaborate. It's REALLY important to have all the devs who are deep in SVG context together.

I'm no longer in the context of SVG optimization and not going to do any significant amount of work by myself for SVGO for sure, but I'll be happy to help with any tech questions and discussions.

@deepsweet
Copy link
Member

deepsweet commented Aug 17, 2017

Yeah, and one more thing: kill the damned COA please regardless the decision you are going to make.

@caub
Copy link
Contributor Author

caub commented Aug 18, 2017

Thanks, interesting

  • I'd like to change plugins signature, someone else already wanted to do that. I'd like to call a plugin with (svgElement, globalOptions, pluginOptions) where globalOptions contain the precision, .. and other common params, and plugin Options the options for this plugin when the user specify them. (it'll simplify config.js), and also avoid the type (perItem, full, perItemReverse) in plugins, It's clear to have this, but the benefit isn't huge I think, I'd simply let plugins require a treeWalk function and do what they need. It'll be more clear I think

  • modularize each plugin is a great idea, more clear to pack tests in each plugin folder. I'm not experienced with monorepos though. Other tricky thing is to define plugins order, I don't really know the best way, either current way, with this hard-coded config but if you make a new plugin, you need to remember to add it (there could be a warning for undeclared plugins). Or maybe, plugins order could be more done from each plugins, where they could declare after: ['moveElemsAttrsToGroup'] or after: 'cleanup' /*after cleanup phase*/ but this is a vague idea, first way makes more sense for now.

Agreed for all, maybe just babel is something that can be separate, it'll be easy to bundle svgo with babel from any other app. And I'd try to avoid cheerio probably too, I don't think it'll make things easier.

I can start this work, as a big pull request or a separate branch.
Also the svgo.optimize would become synchronous (no longer async) since jsdom is sync, it's possible to keep an async version just for compatibility if that's important.

I'm aware of plugins complexity, the most complex and efficient one is probably convertPathData (+_path.js), about 2k LoC

@deepsweet
Copy link
Member

svgo.optimize would become synchronous

It's sync even now in fact, under the hood :) We don't need to wait for SAX in async way actually.

Other tricky thing is to define plugins order

Oh, you just reminded me what I was trying to do. So, the idea (I had a prototype for this as well...):

Each plugin could fire an event, like "hey, I just changed an attribute for this node". Each plugin could subscribe to events, like "hey, I'd like to run again on this node because I know how to remove nodes with specific attributes". Something like Mutation Observer but afaik it's not implemented in JSDOM (yet?) so it requires some wrappers around DOM API helpers.

The madness could be started manually by SVGO like "hey, here is a root node, GO" and then some of plugins should react.

Pros:

  • No plugins order, it just doesn't matter anymore
  • No multipass or any other (hacky) workaround, SVG is always optimized by maximum possible amount of plugin runs
  • Much easier new plugin drop in, just add it

Questions:

  • Is it possible at all? Circular dependencies?

WDYT?

@deepsweet
Copy link
Member

(svgElement, globalOptions, pluginOptions)

(svgElement, { ...globalOptions, ...pluginOptions })?

@caub
Copy link
Contributor Author

caub commented Aug 18, 2017

Yes right, makes more sense to merge options, noted

ah MutationObserver could be interesting, but yes first jsdom doesn't seem to support it and it would not be very easy, it looks close to AI, I don't know well enough all plugins and how it would spread.

I think most plugins are standalone, really independant: all cleanupXY, removeXY, minifyStyles, sortAttrs, ... ones
There is the transform suite (moveElems.., moveAttrs.., collapseGroups, convertTransform )
and the path data ones (hardest ones): convertPathData, mergePaths, convertShapeToPath

I'd like also to simplify a bit, are all options (except the precisions) really useful in: https://github.com/svg/svgo/blob/master/plugins/convertTransform.js#L10-L21, except for testing? (when looking the 4 tests files, they are all with default options)

I'll just try to get familiar with PathData a bit more first, then when it's ready start the work on svgo the way you think is better, also I'd like to use https://github.com/jarek-foksa/path-data-polyfill.js which will become SVG2 spec

edit: oh I see why you mentioned babel, I use recent node versions, I forgot about old ones, it's always more annoying to do, but rollup+babel is practical for that

in priority:

  • adapt all plugins with jsdom
  • ... ?
  • test the way with modularized plugins (it is overkill for very short ones?)

@twardoch
Copy link

And please make it work with pkg so it can be turned into standalone executables.

pkg chokes on the svgo dynamic plugin loading method and on some coa stuff, so I had to hack around when making my standalone executable svgop version — more at #783

@GreLI
Copy link
Member

GreLI commented Sep 14, 2017

Well, if @deepsweet agrees (he is, as far as I see), I don't mind either and ready to give permissions to @caub (unless @deepsweet'll do it earlier 😀, I'm working on SVGO from time to time).

Just to be in sync: I'm planning to manage plugins (make more optimization settings, remove outdated editing ones), fix some dumb bugs and then release v1.0.0. (I've already bumped Node version to 4, so it's safe to use supported features like let, const, arrow functions, for-of, generators, classes, Symbols, Promises, but not (yet) destructuring, rest/spread parameters, Proxy.)

Releasing v1.0.0 will allow normal Semver releases with faster pace. If you want to do something big, it's better to make a separate branch, just not to block minor releases.

@caub
Copy link
Contributor Author

caub commented Sep 14, 2017

Ok, nice

I think jsdom will be an interesting turn, there's just one thing I'd like to improve on jsdom it's .getComputedStyle (currently it doesn't work with inherited attrs), to have something like the current .computedAttr in svgo, but more powerful

example here: stroke could be defined in a style attribute, or even in a <style>, getComputedStyle would work for those cases, not just as an attribute

I don't know if that's a good idea?

@GreLI
Copy link
Member

GreLI commented Sep 15, 2017

Yep, I've introduced computedAttr thinking in that way.

@caub
Copy link
Contributor Author

caub commented Oct 22, 2017

I'm just temporarily closing it, jsdom is moving in the right way, with better svg support and a working window.getComputedStyle, but it's moving slow. Will reopen when I can finish the porting

@caub caub closed this as completed Oct 22, 2017
@caub
Copy link
Contributor Author

caub commented Nov 1, 2017

@strarsis your efforts are great, can you maybe help later to convert all your recent additions into the proposal here (which is using jsdom as parser), so for for example no need for a CSSStyleDeclaration plugin (it's in document.stylesheets, or styleElement.style), getComputedStyle can be used too, etc..

It'll be a bunch of work to convert all plugins to the DOM API, It's great if some of us can work on an alpha branch, and plan the release of jsdom for svgo 2.x?

Plugins have often untested options, sometimes more or less useful (ex: https://github.com/svg/svgo/blob/master/plugins/convertTransform.js#L14-L21) I'm a bit afraid to break things in the conversion

Do we start putting tests in plugins also for this release? as a step toward standalone plugins / monorepo (discussed previously)

@caub caub reopened this Nov 1, 2017
@strarsis
Copy link
Contributor

strarsis commented Nov 1, 2017

@caub: Yes, migrating to jsdom is the logical consequence in my opinion,
it offers a great base for handling DOM and styles.
Also window.getComputedStyle(...) offers a much simpler and more elegant approach (besides possible caching/performance issues) than the current inlineStyles implementation.

liftoff is another nice library for adding *file and plugin infrastructure.
Maybe like in postcss where plugin can be installed by npm (e.g. my usedChars plugin).

IMHO the most important part of svgo are actually the tests :)
They evolved over time and successfully take the large amounts of different SVGs eachday into account.

@ghost
Copy link

ghost commented Aug 18, 2019

Hello, everyone!

Sorry for necrobumping, but I would like to be able to let y’all know that I have interest in this.

In fact, yesterday, I have written my own “version” of SVGO using the DOM (JSDOM on Node) that is compatible with existing SVGO plugins.

Today I have spent the day thinking about and experimenting with this “event‐driven” plugin idea, but I couldn’t come up with anything concrete, unfortunately. I do believe that I’m just being dense, though.

So if anyone wouldn’t mind helping come up with a more concrete idea about how to use events to implement a plugin engine, I’d love to be able to help implement it!

I find it to be really unfortunate that SVGO seems to be going unmaintained, as there is no other alternative that’s currently maintained either. (Both scour and svgcleaner haven’t had any activity in about a year.)

@TrySound
Copy link
Member

JSDOM is quite heavy thing for parsing xml. Better focus on something smaller.
https://packagephobia.com/result?p=jsdom

@caub
Copy link
Contributor Author

caub commented Feb 13, 2021

@TrySound agreed, but the idea was to be able to parse css, and not just parse inline style (I believe svgo still only use inline styles), and have a getComputedStyles helper. https://github.com/Zirro/css-object-model could help

@strarsis
Copy link
Contributor

strarsis commented Feb 13, 2021

@caub: Well, svgo already got some CSS support. Adding its own getComputedStyles function would help, right?

@caub
Copy link
Contributor Author

caub commented Feb 14, 2021

@strarsis yea, something close to getComputedStyle (but maybe a simpler version) would be ideal, because computedAttr is quite limited https://github.com/svg/svgo/search?q=computedAttr stroke and fill could be passed in inline style attribute, or in a a <style> declaration

@TrySound
Copy link
Member

TrySound commented Mar 2, 2021

Btw jsdom would not work because it does not consider inheritance which may be also significant factor.

TrySound added a commit that referenced this issue Mar 2, 2021
Ref #777

Currently a lot of optimisations are attributes specific and may be
broken because of inline or shared styles.

In this diff I'm trying to solve the problem with getComputedStyle
analog.

`computeStyle` collects attributes, shared css rules, inline styles
and inherited styles and checks whether they can be statically optimised
or left as deoptimisation.
@caub
Copy link
Contributor Author

caub commented Mar 3, 2021

yea ok, anyway I see you're making great changes @TrySound, hence I think you could close this issue if you want. Thanks for doing the things I wasn't able to hah

TrySound added a commit that referenced this issue Mar 3, 2021
Ref #777

Currently a lot of optimisations are attributes specific and may be
broken because of inline or shared styles.

In this diff I'm trying to solve the problem with getComputedStyle
analog.

`computeStyle` collects attributes, shared css rules, inline styles
and inherited styles and checks whether they can be statically optimised
or left as deoptimisation.
TrySound added a commit that referenced this issue Mar 4, 2021
Ref #777

Currently a lot of optimisations are attributes specific and may be
broken because of inline or shared styles.

In this diff I'm trying to solve the problem with getComputedStyle
analog.

`computeStyle` collects attributes, shared css rules, inline styles
and inherited styles and checks whether they can be statically optimised
or left as deoptimisation.
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

No branches or pull requests

6 participants