impro port #92
impro port #92
Conversation
This removes special casing for the -flip argument and instead makes the logic work correctly for any such options.
The ignoreAspectRatio and withoutEnlargement options must come after a resize operation in the new library, so reorder the query string if need be to ensure they are applied correctly by sharp.
In order to know ahead of time (i.e. suitable for being a header) the set of used engines, we need to trigger a pipeline flush() so that the batching and selection is performed. Do this and calculate the ordered set of name engines based on the recorded usedEngines.
Opt to append the comma during prepare to get the correct behaviour.
This restores all but one of the Gifsicle unavailable tests.
The solution to them will be the same one and will be simpler to address once we reach the end of the suite.
Between the duplex stream of the pipeline and hijackresponse there was a situation causing an errant end event to cancel the streams just prior to fully handling an error error event. This is somehow reminiscent of another situation where having bidirectional streams in the mix created confusion. Attempt to get around this by using pipe() in only one direction and in the other handing the events by hand.
There was a problem hiding this comment.
This looks really promising!
I don't really use this module for a lot of things, except https://github.com/assetgraph/assetgraph-builder/blob/0fe7fa16dd0e46fe77e6ef833a4162d99b2b3b36/lib/transforms/processImages.js#L13, which will also have to be ported over to use impro directly.
I guess your $work is the main stakeholder here, so I'm happy to let you run with it :)
Co-Authored-By: Andreas Lind <andreaslindpetersen@gmail.com>
|
@papandreou thanks very much for the review. I confess I've done this more as a means of battle hardening I'll pick up your comments on this and try to get this merge ready! |
|
@alexjeffburke, yeah, it should be hugely beneficial. We should totally aim to get this landed somehow. |
This reverts commit 0b3c0eb.
|
This is now very close. The only thing that needs being tracked down is the slight output change that I've marked in a couple of places, one example of which is https://github.com/papandreou/express-processimage/pull/92/files#diff-66f183145d1ddbe3b535da2849a60091R286 My immediate guess is that e-p doesn't issue sharp resize with its default mode or something, but could do with another pair of eyes to peek at it. |
|
This should be ready for merge :) |
This PR attempt to replace the image processing code in this module with the
impromodule.The module itself is a heavily restructured and reworked version of the processing code originally extracted from this repository. In addition, the image processing code as-is here is showing is very difficult to maintain and certainly to extend.
This port was an attempt to allow moving forward with processimage. In addition, it would be an important step in evolving the module and proving it is mature enough to support all the use-cases represented by this middleware.
Notes
The approach taken is to convert URLs handled by
express-processimageinto their new equivalents, but to do so in a backwards compatible way so this can be dropped in to existing projects.Currently, the PR is complete enough to pass the entire test suite but there is still some amount of cleanup that is required and a couple of output differences that need to be investigated - see commits 0b3c0eb and 2c90f35.