search

papandreou / express-processimage

Express middleware that processes served images according to the query string
BSD 3-Clause "New" or "Revised" License
56 stars 18 forks source link

Missing fallback from sharp to gm #54

Closed alexjeffburke closed 6 years ago

alexjeffburke commented 6 years ago

Hi,

I stumbled on a case that is not correctly falling back from sharp to gm - failing test is here.

What seems to be happening is the second 'embed' argument causes the first setFormat=gif to be incorrectly attempted on a sharp instance which fails. The test can be made to pass by changing the || at https://github.com/papandreou/express-processimage/blob/master/lib/getFilterInfosAndTargetContentTypeFromQueryString.js#L512 to an &&, but that causes other defaulting to go awry and other tests break.

I've tried a good few other things, and I can get it close but there's aways something that breaks. My gut says the right solution might actually be to bite the bullet and attach the correct engineName to each operation, which would also pave the way to mixing and matching filters - but that's a much larger change. Opening an issue as the defaulting logic is a little fragile and my attempt to be minimal haven't succeeded so far so I can probably do with some pointers.

Let me know what else I can provide.

alexjeffburke commented 6 years ago

Hah, so a little after I open an issue I end up with something that passes all the tests.

I think the logic could use some review but I have opened a PR for it: https://github.com/papandreou/express-processimage/pull/55

papandreou commented 6 years ago

Oh, I didn't realize you were still working on this :(

In the mean time I also came up with a fix, published in 8.0.2. Could you see if that works?

alexjeffburke commented 6 years ago

Oh no worries, I thought I'd fiddle with it one last time though I still wouldn't say I was totally confident with what I did. I can't see the fix on master but I'll try the new package next time I sit win it. Also, thanks for looking at it! :)

alexjeffburke commented 6 years ago

Ah crap, also I'm sorry about the mix-up with my email addy - that was supposed to be a personal contribution done in own time. Will make sure that doesn't occur again.

papandreou commented 6 years ago

What’s that about the email address? This project doesn’t have a policy that prohibits company contributions, so no worries 😂

papandreou commented 6 years ago

Ah, forgot to push, should be there now :)