unjs / ipx

🖼️ High performance, secure and easy-to-use image optimizer.
MIT License
1.54k stars 61 forks source link

Ability to change the modifiers' separator #57

Closed lucien144 closed 2 years ago

lucien144 commented 2 years ago

Modifiers' separator is hardcoded as comma. This might be a problem in <picture> tag where comma-separated list can be used in source's srcset to define list of possible images based on size.

Problematic code:

<picture>
    <source srcset="http://localhost:3000/embed,f_webp,s_200x200/static/buffalo.png" type="image/webp">
    <img src="http://localhost:3000/s_200x200/static/buffalo.png">
</picture>

The line <source srcset="http://localhost:3000/embed,f_webp,s_200x200/static/buffalo.png" type="image/webp"> is actually read by browsers like 3 different URLs:

  1. http://localhost:3000/embed
  2. f_webp
  3. s_200x200/static/buffalo.png

Suggestion:

Introduce new env variable IPX_SEPARATOR.

Change this line https://github.com/unjs/ipx/blob/fc8c7b5b655d61e23f6f63af82669ed23e48eec5/src/middleware.ts#L46 to something like this:

  // Read modifiers from first segment
  if (modifiersStr !== '_') {
    for (const p of modifiersStr.split(getEnv('IPX_SEPARATOR', ','))) {
      const [key, value = ''] = p.split('_')
      modifiers[key] = decode(value)
    }
  }

Note:

URL encoded comma %2C won't help in this case.

I'm more than happy to make a PR if you find this solution suitable and wanted.

pi0 commented 2 years ago

Thanks for writing a detailed issue. I think we can introduce more (default) separators since this is a valid enough case to avoid commas. I'm thinking to use & for this like embed&f_webp (For other options: I might introduce : as replacement for _ and also + is less safe as some encoders can potentially consider it as space)

chrisspiegl commented 1 year ago

I tested this just now with v1.0.0-2 and it seems to give an error:

…localhost:3000/image/w_100&format_webp/https://r2-bucket…

Gives error:

IPX Error: Error: Expected positive integer for width but received 100&format of type string

While the version with , as the separator does work?

Am I missing something?

chrisspiegl commented 1 year ago

One more update to the above issue: when I use the url in a <img src="" /> tag, it works with the & separator.

The error above happens in Insomnia (API request testing app like Postman). Not sure what exactly is going on here…

pi0 commented 1 year ago

@chrisspiegl Do you mind to open a new issue to track this? (also a small reproduction step i can quickly check)

chrisspiegl commented 1 year ago

@pi0 I just tried to reproduce in a clean environment by simply running it in the console via pnpm ipx without anything around it and the "issue" I was having with Insomnia was not happening.

I am now guessing that the "Issue" might be coming from the Firebase Emulator and the way the Google Cloud Functions are running things.

I will get back to this once I have tested IPX and my cloud function online at some point in the future.

If it shows up there as well, I will get back to you and open a dedicated issue. Otherwise, let's consider this "non existent for now" 🙈.