New URL structure #24

martijngastkemper · 2023-05-08T16:30:58Z

This is an exciting case for the image scaler: double extensions which are both an allowed output type: foo.jpg.jpg. This forces the original file (foo.jpg) to be converted to JPG. But the original file is foo.jpg.jpg. I don't think we have a way to determine the original file name. It could be both.

Let's discus.

harmenjanssen · 2023-05-09T04:56:49Z

Is this a real-world case?
I don't see how the image scaler would be able to determine the original filename in this case, without explicitly looking up multiple options on S3. This would also be necessary when the original would be foo.jpg.png.

One possible scenario could be to always look up the entire path to see if it exists, and if so, determine that the original file is requested without conversion.
If not, chop off the last extension and try again.

It will make all requests slower however... (it will also be harder to unit test because this logic is currently separate from looking up anything on S3, but that's another problem).

martijngastkemper · 2023-06-30T13:30:05Z

I've put it together. Probably a lot to improve, but I think it's a good moment for a first review. Naming must be checked. And I've replaced the pipe with an underscore. The pipe caused an error somewhere in AWS, but the moment the pipe was in the URL the Lambda function isn't even executed.

I've a deploy version (MLK staging) for testing: https://7ij2wddzl7.execute-api.eu-west-1.amazonaws.com/staging/resize?key=/scaled/convert:webp_height:100/mock/event-1.jpg.webp

harmenjanssen

Nice work! Looking good, nice to clean some things up.

I'm having trouble reading the getInputFormat and getOutputFormat functions. Not because they're written unclearly, but because that stupid parameter just throws me off.

I'll try to think of a way to make this more explicit.

CHANGELOG.md

README.md

test/cleanupPath.test.js

test/getImageMimeType.test.js

harmenjanssen · 2023-07-03T06:42:20Z

test/getInputFormat.test.js

+it("Return the right input format", function () {
+  expect(getInputFormat("foo.jpg")).toBe("jpg");
+  expect(getInputFormat("foo.jpg.png")).toBe("png");
+  expect(getInputFormat("foo.jpg.png", "png")).toBe("jpg");


Hmm, this test makes me realize I find the signature of getInputFormat very difficult to read. This assertion was really surprising to me.
But I understand how it works when combined with the options string. I'll try to think of a way to make this API a little clearer.

Did you come up with something? I tried to find another name, but keep on circling around "format" and "extension". They are both the same, but getInputExtension isn't better IMO. Or maybe something with "source" and "destination" to ditch the word "input" might do the trick.

No, let's keep it like this.
Maybe in the future I'll find some inspiration.

test/resizeImage.test.js

util/getImageMimeType.js

util/parseOptions.js

harmenjanssen · 2023-07-03T06:58:24Z

util/resizeImage.js

+
+  const sharp = SHARP(body).withMetadata().resize(resizeOptions);
+
+  if (outputFormat) {


Is outputFormat ever undefined?

It's a function argument.

harmenjanssen

LGTM!

martijngastkemper · 2023-07-21T14:35:49Z

@harmenjanssen Ik krijg die tests niet werkend op Ubuntu, lokaal wel. In de README staat iets over shard. Ik heb geprobeerd npm_config_platform en npm_config_arch aan te passen, maar dat veranderd bij mij lokaal niets. Weet jij hoe die fixtures kunnen worden geupdate of heb je een suggestie waar ik dat kan zoeken.

harmenjanssen · 2023-07-24T11:29:31Z

Hmm, vaag.
Ik heb Sharp dus destijds toegevoegd aan de lock file dmv deze regel:

npm_config_platform=linux npm_config_arch=x64 yarn add sharp

Dit zodat de binaries gebuild worden als Linux x64 versies.
Ik zie dat deze PR Sharp wel upgrade naar een nieuwe versie, maar is dat wel weer met een dergelijke one liner gebeurd? Anders zit daar misschien het probleem. En het kan sowieso natuurlijk wel zijn dat de nieuwe versie net een iets andere JPEG genereert en dat de fixtures misschien geupdate moeten worden?

- Add support for parameters other then width and height - 500x500 becomes widht:500|height:500 - Add convert parameter to solve the issue with double extensions in the original file. (the reason this change has been made). - Upgrade Jest - Update README.md - Add CHANGELOG.md

Image created on Linux aren't the same as on MacOS. So when creating fixtures you have to do that on MacOS.

martijngastkemper · 2023-07-25T17:30:02Z

I really don't understand how the test fixtures have ever worked cross-platform. I've fixed it by running the tests on MacOS runners. Developers working on Linux have a problem, but I invite them to come up with a better solution.

martijngastkemper · 2023-07-25T17:34:45Z

@harmenjanssen Can you look at the last commit, changing to a MacOS runner.

harmenjanssen

I've thought about this, and came to the following conclusion: when I started this project I deployed from local to Lambda using Serverless.
That meant my local binaries were shipped to AWS and quickly turned out to be incompatible.

That's when I found that one-liner that forces the Linux versions to be installed, even when on MacOS. (and apparently this was no problem when running this on MacOS)
This way I could ship the correct binaries to AWS from my local machine.

However, I think the upgrade in Sharp introduced an incompatibility. I also see you changed some of the fixtures in this PR. Not sure why that is, did you re-generate them?

In any case: I'm totally fine with running the tests on a MacOS runner.
As long as we the tests are consistent between developer machines and CI, it's fine.
As long as a working version is deployed to AWS, it's fine.
It's too bad you can't deploy from your local machine, but that's not a hard requirement. (and maybe Serverless has a solution for this, come to think of it. Incompatible binaries are not uncommon in NPM land)

martijngastkemper · 2023-07-26T12:46:37Z

However, I think the upgrade in Sharp introduced an incompatibility. I also see you changed some of the fixtures in this PR. Not sure why that is, did you re-generate them?

Yes because my tests failed locally.

It's too bad you can't deploy from your local machine, but that's not a hard requirement. (and maybe Serverless has a solution for this, come to think of it. Incompatible binaries are not uncommon in NPM land)

Changing platfrom and arch is the way to go: https://sharp.pixelplumbing.com/install#cross-platform
But this doesn't work for running code. Because it's not possible to run a macOS binary on Linux. So deploying from an incompatible OS is always tricky.

I'm going to merge this PR. It has taken way to much time. Maybe we find a better solution in the future.

martijngastkemper requested a review from harmenjanssen May 8, 2023 16:30

martijngastkemper force-pushed the feature/martijn.g-issue-with-double-extensions branch 3 times, most recently from 399c2f3 to 80c536f Compare June 30, 2023 13:22

martijngastkemper changed the title ~~Add failing test~~ New URL structure Jun 30, 2023

martijngastkemper force-pushed the feature/martijn.g-issue-with-double-extensions branch from 80c536f to c4b5ccf Compare June 30, 2023 13:26

harmenjanssen reviewed Jul 3, 2023

View reviewed changes

martijngastkemper marked this pull request as ready for review July 7, 2023 14:38

martijngastkemper requested a review from harmenjanssen July 7, 2023 14:38

harmenjanssen approved these changes Jul 12, 2023

View reviewed changes

martijngastkemper force-pushed the feature/martijn.g-issue-with-double-extensions branch from 3589d3f to c0fabeb Compare July 12, 2023 10:10

martijngastkemper changed the base branch from v3.x to v4.x July 12, 2023 10:14

martijngastkemper added 2 commits July 25, 2023 19:26

Run tests on MacOS

faf43a7

Image created on Linux aren't the same as on MacOS. So when creating fixtures you have to do that on MacOS.

martijngastkemper force-pushed the feature/martijn.g-issue-with-double-extensions branch from 6c5d263 to faf43a7 Compare July 25, 2023 17:27

martijngastkemper requested a review from harmenjanssen July 25, 2023 17:34

harmenjanssen approved these changes Jul 26, 2023

View reviewed changes

martijngastkemper merged commit c516e79 into v4.x Jul 26, 2023
4 checks passed

martijngastkemper deleted the feature/martijn.g-issue-with-double-extensions branch July 26, 2023 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New URL structure #24

New URL structure #24

martijngastkemper commented May 8, 2023

harmenjanssen commented May 9, 2023

martijngastkemper commented Jun 30, 2023

harmenjanssen left a comment

harmenjanssen Jul 3, 2023

martijngastkemper Jul 7, 2023

harmenjanssen Jul 10, 2023

harmenjanssen Jul 3, 2023

martijngastkemper Jul 7, 2023

harmenjanssen left a comment

martijngastkemper commented Jul 21, 2023

harmenjanssen commented Jul 24, 2023

martijngastkemper commented Jul 25, 2023

martijngastkemper commented Jul 25, 2023

harmenjanssen left a comment

martijngastkemper commented Jul 26, 2023


		const sharp = SHARP(body).withMetadata().resize(resizeOptions);

		if (outputFormat) {

New URL structure #24

New URL structure #24

Conversation

martijngastkemper commented May 8, 2023

harmenjanssen commented May 9, 2023

martijngastkemper commented Jun 30, 2023

harmenjanssen left a comment

Choose a reason for hiding this comment

harmenjanssen Jul 3, 2023

Choose a reason for hiding this comment

martijngastkemper Jul 7, 2023

Choose a reason for hiding this comment

harmenjanssen Jul 10, 2023

Choose a reason for hiding this comment

harmenjanssen Jul 3, 2023

Choose a reason for hiding this comment

martijngastkemper Jul 7, 2023

Choose a reason for hiding this comment

harmenjanssen left a comment

Choose a reason for hiding this comment

martijngastkemper commented Jul 21, 2023

harmenjanssen commented Jul 24, 2023

martijngastkemper commented Jul 25, 2023

martijngastkemper commented Jul 25, 2023

harmenjanssen left a comment

Choose a reason for hiding this comment

martijngastkemper commented Jul 26, 2023