Refactor the processImages, optimizePngs and optimizeJpegs transforms

[papandreou]

Ideas:

• Merge into one transform.
• Use node-pngquant, node-pngcrush, node-optipng, and node-jpegtran instead of the horrible pipeImageThroughChildProcessAndBuffer thing.
• The "automatic" optimizations in transforms.optimizePngs and transforms.optimizeJpegs shouldn't kick in if the same tool is already specified explicitly in the query string.

@Munter, what do you think?

[Munter]

It's obvious that we should refactor the optimization transforms to use the new stream based libraries.

I also very much like the idea of not having to rerun optimizations in case some of them have already been applied in an explicit step earlier. Running all those optimizations is heavy work, and the less we do of it the better.

[Munter]

On the idea of merging all of these steps into one I am not quite certain it is a good idea.

Mostly because is the intermediate spriting step. If we find a way to make that fit in there seamlessly I think it's a good approach.

[papandreou]

It's mostly because of the need to avoid rerunning the optimizations that I think it should be one step. I've tried very hard to avoid maintaining any shared state between the transforms. Maybe a compromise could be to rely on the suffixes that are already added to the file names of the images that have been processed by transforms.processImages.

Wrt. the spriting, yeah. You might want to process an image before it's sprited and process the sprite itself afterwards. Maybe running transforms.processImages twice could be an option and only do the automatic optimizations in one of the runs. Except for the automatic optimizations it would still be "idempotent" in the sense that it removes the processing instructions from the query string after they've been performed.

[papandreou]

How about something like this? One-com@e041bb7

Not yet implemented assumptions:

• After spriting the processing instructions for the sprite image will be in the query string
• An image will be sprited if it has a sprite param in its query string

[Munter]

This is great!

Given those assumptions the only thing left I can think of is spriting post processing instruction conflict handling that might cause usability issues with developers.

I think we can put it in the wild without a clear strategy on that single issue though. It doesn't seem to be a widespread use case given that not many people are even aware if the spriting ability. Also this solution doesn't do any worse than the previous one with vendor specific css properties, where you could also add post processing conflicts.

[papandreou]

I'm not exactly sure what you mean by "spriting post processing instruction conflict handling"?

But yeah, we need to invent syntax for specifying what is to be done to the sprite after it's generated.

This would be sufficient:

.icon {
    -ag-selector-for-group: icons;
    -ag-sprite-query-string: pngquant=40&optipng=-o7;
}

But it might be cooler to mirror the syntax for background / background-image. Then we could get rid of things like -ag-sprite-important and probably avoid introducing more -ag-sprite-... properties in the future:

.icon {
    -ag-selector-for-group: icons;
    -ag-sprite-background-image: url(mySprite.png?pngquant=40&optipng=-o7) !important;
}

Then AssetGraph-sprite could just promote -ag-sprite-background-image to background-image (or -ag-sprite-background to background) and attach the relation to that. The file name part of the url(...) token could be optional.

Hmm, maybe it'd even be possible to strip the -ag-sprite- prefix for those properties if we made Css.findOutgoingRelationsInParseTree skip this (and if it turns out that browsers still apply red and !important in development mode):

.icon {
    -ag-selector-for-group: icons;
    background: red url(?pngquant=40&optipng=-o7) !important;
}

[papandreou]

Ok, background: url(?pngquant=40) and background: url(#?pngquant=40...) won't work. The browser simply fetches the containing css file with the query string and/or fragment identifier added.

That leaves this as my preferred option:

.icon {
    -ag-selector-for-group: icons;
    -ag-sprite-background-image: url(mySprite.png?pngquant=40&optipng=-o7) !important;
}

[Munter]

How about vendor prefixed selectors?

-ag-sprite spriteSheetName {
    selector: icons;
    background: url(pngquant=40&optipng=-o7);
}

I haven't tried this. But is seems better to me if we can make a css block per sprite sheet and use that to specify post processing and other specific sheet relevant sprite options.

If my intuition is right, browsers should ignore all of this, since the selector is unknown. Thus not adding any problems with the invalid background url.

[papandreou]

Hmm, interesting... Although part of the idea is that additional properties in the group selector should get applied in "development mode", like:

.icon {
    float: left;
    -ag-sprite-selector-for-group: icons;
    [...]
}

... and in the example with background: red... it was also my intention that all elements matched by the group selector would get a red background in development mode.

So it's really only the url that's the problem. Maybe we should bite the bullet and add that -ag-sprite-url property. It could still be combined with an existing background or background-image property:

.icon {
    -ag-sprite-selector-for-group: icons;
    -ag-sprite-url: allIcons.png?pngquant=256;
    background: red !important;
}

... which would compile to (again, the file name part could be omitted):

.icon {
    background: red url(allIcons.png?pngquant=256) !important;
}

We would still get rid of -ag-sprite-important, so it'd be status quo as far as the number of obscure options is concerned.

[Munter]

Yeah, -ag-sprite-url is probably the best solution then.

I'd like to see how many of the vendor specific properties we can live without. And if that frees up something, maybe shorten the ones left over.

[papandreou]

All right!

I guess it has to be -ag-sprite-location: url(allIcons.png?pngquant=256) to avoid introducing new delimiters -- let's say somebody wanted the the query string to contain a semicolon.

[Munter]

The only thing I don't like about that is the property naming.
Could we name the postprocessing instructions something that sounds more like post processing? Filter? Process? Tasks?

We could also drop using -ag- as vendor prefix and just use -sprite- instead. Since we have no property just called -ag-sprite there would be no name clash. That would make it look less noisy

[papandreou]

Implemented -ag-sprite-location and the other things we talked about (plus much better test coverage) and updated the docs: https://github.com/One-com/assetgraph-sprite

The custom CSS property previously known as -ag-no-group-selector became the CGI parameter spriteNoGroup, which I'm not entirely happy about.

Could we name the postprocessing instructions something that sounds more like post processing? Filter? Process? Tasks?

-ag-sprite-location is more general than that. It can be used to specify the desired url of the generated sprite, not just the image processing instructions.

About switching from -ag-sprite- to -sprite-, well yeah. What would -ag-image-inline become? Ah, a GET parameter I suppose?

[Munter]

Seems to me that every source images modifications can be expressed through the GET parameter chain, which then ends in an optional sprite call.

The only reason sprites need this css block is that they don't have reference to define a similar chain of parameters to.

So given this assumption is true, only the sprite specific stuff needs vendor prefixed css properties.

I don't know what -ag-no-group-selector does. Since I can't infer it from the name it definitely does need a name change :)

[papandreou]

spriteNoGroup alias -ag-no-group-selector means "even if there is a group selector for the sprite group I belong to I want to keep an explicit background-image pointing at the sprite image", for example:

.mySprite {
    -ag-sprite-group-selector-for: mySprite;
}
.foo {
    background-image: url(foo.png?sprite=mySprite);
}
.bar {
    background-image: url(bar.png?sprite=mySprite&spriteNoGroup);
}

... which becomes:

.mySprite {
    background-image: url(123.png);
}
.foo {
    background-position: 0 0;
}
.bar {
    background-image: url(123.png);
    background-position: -20px 0;
}

This is for scenarios where you can't guarantee that an element is matched by both .mySprite and .bar, but you still want to add bar.png to the sprite.

[papandreou]

Dropped -ag in spriting instructions: assetgraph/assetgraph-sprite@ba2e8fd
Changed transforms.inlineCssImagesWithLegacyFallback to use a GET parameter: assetgraph/assetgraph@3b76806

[papandreou]

Landed on master in f42572a.

[Munter]

Can't wait to try all of this out. It seems like a massive improvement and simplification.
It's going to be interesting to present this and hear what people will think about it.

[papandreou]

Yeah, I've only played a little with it in LiveStyle, but it seems very powerful and flexible.

[papandreou]

Deprecated --optimizejpegs and --optimizepngs in favor of --pngquant, --pngcrush, --optipng, --jpegtran, and --optimizeimages (turns on all of them). Added a little docs to the README. Released as part of assetgraph-builder 1.3.0.