Skip to content

Commit

Permalink
fix: Disabling image optimize breaks image path (#3931)
Browse files Browse the repository at this point in the history
## Description

closes  #3754

## Steps for reproduction

1. click button
2. expect xyz

## Code Review

- [ ] hi @kof, I need you to do
  - conceptual review (architecture, feature-correctness)
  - detailed review (read every line)
  - test it on preview

## Before requesting a review

- [ ] made a self-review
- [ ] added inline comments where things may be not obvious (the "why",
not "what")

## Before merging

- [ ] tested locally and on preview environment (preview dev login:
5de6)
- [ ] updated [test
cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md)
document
- [ ] added tests
- [ ] if any new env variables are added, added them to `.env` file
  • Loading branch information
istarkov authored Sep 9, 2024
1 parent 54b6fd0 commit df5a8e8
Show file tree
Hide file tree
Showing 7 changed files with 77 additions and 15 deletions.
4 changes: 3 additions & 1 deletion fixtures/ssg/app/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
* We use mjs extension as constants in this file is shared with the build script
* and we use `node --eval` to extract the constants.
*/
import { UrlCanParse } from "@webstudio-is/image";

export const assetBaseUrl = "/assets/";
export const imageBaseUrl = "/assets/";

/**
* @type {import("@webstudio-is/image").ImageLoader}
*/
export const imageLoader = ({ src }) => {
if (URL.canParse(src)) {
if (UrlCanParse(src)) {
return src;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/cli/templates/ssg-netlify/app/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
* We use mjs extension as constants in this file is shared with the build script
* and we use `node --eval` to extract the constants.
*/
import { UrlCanParse } from "@webstudio-is/image";

export const assetBaseUrl = "/assets/";
export const imageBaseUrl = "/assets/";

/**
* @type {import("@webstudio-is/image").ImageLoader}
*/
export const imageLoader = (props) => {
if (URL.canParse(props.src)) {
if (UrlCanParse(props.src)) {
return props.src;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/cli/templates/ssg-vercel/app/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
* We use mjs extension as constants in this file is shared with the build script
* and we use `node --eval` to extract the constants.
*/
import { UrlCanParse } from "@webstudio-is/image";

export const assetBaseUrl = "/assets/";
export const imageBaseUrl = "/assets/";

/**
* @type {import("@webstudio-is/image").ImageLoader}
*/
export const imageLoader = (props) => {
if (URL.canParse(props.src)) {
if (UrlCanParse(props.src)) {
return props.src;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/cli/templates/ssg/app/constants.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@
* We use mjs extension as constants in this file is shared with the build script
* and we use `node --eval` to extract the constants.
*/
import { UrlCanParse } from "@webstudio-is/image";

export const assetBaseUrl = "/assets/";
export const imageBaseUrl = "/assets/";

/**
* @type {import("@webstudio-is/image").ImageLoader}
*/
export const imageLoader = ({ src }) => {
if (URL.canParse(src)) {
if (UrlCanParse(src)) {
return src;
}

Expand Down
20 changes: 10 additions & 10 deletions packages/image/src/image-loaders.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,19 @@ export const createImageLoader =
return src;
}

// const searchParams = new URLSearchParams();
if (format !== "raw") {
resultUrl.searchParams.set("width", width.toString());
resultUrl.searchParams.set("quality", quality.toString());

resultUrl.searchParams.set("width", width.toString());
resultUrl.searchParams.set("quality", quality.toString());
resultUrl.searchParams.set("format", format ?? "auto");

if (props.format !== "raw" && props.height != null) {
resultUrl.searchParams.set("height", props.height.toString());
}
if (props.height != null) {
resultUrl.searchParams.set("height", props.height.toString());
}

if (props.format !== "raw" && props.fit != null) {
resultUrl.searchParams.set("fit", props.fit);
if (props.fit != null) {
resultUrl.searchParams.set("fit", props.fit);
}
}
resultUrl.searchParams.set("format", format ?? "auto");

resultUrl.pathname = joinPath(resultUrl.pathname, encodePathFragment(src));

Expand Down
38 changes: 38 additions & 0 deletions packages/image/src/image-optimize.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,26 @@ describe("Image optimizations applied", () => {
`);
});

test("width is number, url is absolute, create pixel density descriptor 'x'", () => {
const imgAttr = getImageAttributes({
optimize: true,
width: 100,
src: "https://webstudio.is/logo.webp",
srcSet: undefined,
sizes: undefined,
quality: 100,
loader: createImageLoader({ imageBaseUrl: "/asset/image/" }),
});

expect(imgAttr).toMatchInlineSnapshot(`
{
"sizes": "100vw",
"src": "/asset/image/https%3A//webstudio.is/logo.webp?width=256&quality=100&format=auto",
"srcSet": "/asset/image/https%3A//webstudio.is/logo.webp?width=16&quality=100&format=auto 16w, /asset/image/https%3A//webstudio.is/logo.webp?width=32&quality=100&format=auto 32w, /asset/image/https%3A//webstudio.is/logo.webp?width=48&quality=100&format=auto 48w, /asset/image/https%3A//webstudio.is/logo.webp?width=64&quality=100&format=auto 64w, /asset/image/https%3A//webstudio.is/logo.webp?width=96&quality=100&format=auto 96w, /asset/image/https%3A//webstudio.is/logo.webp?width=128&quality=100&format=auto 128w, /asset/image/https%3A//webstudio.is/logo.webp?width=256&quality=100&format=auto 256w",
}
`);
});

test("width is undefined, create 'w' descriptor and sizes prop", () => {
const imgAttr = getImageAttributes({
optimize: true,
Expand Down Expand Up @@ -111,6 +131,24 @@ describe("Image optimizations applied", () => {

describe("Image optimizations not applied", () => {
test("optimize is false", () => {
const imgAttr = getImageAttributes({
optimize: false,
width: 100,
src: "logo.webp",
srcSet: undefined,
sizes: undefined,
quality: 100,
loader: createImageLoader({ imageBaseUrl: "/asset/image/" }),
});

expect(imgAttr).toMatchInlineSnapshot(`
{
"src": "/asset/image/logo.webp?format=raw",
}
`);
});

test("optimize is false and url absolute", () => {
const imgAttr = getImageAttributes({
optimize: false,
width: 100,
Expand Down
18 changes: 17 additions & 1 deletion packages/image/src/image-optimize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,18 @@ const DEFAULT_SIZES = "(min-width: 1280px) 50vw, 100vw";

const DEFAULT_QUALITY = 80;

/**
* URL.canParse(props.src)
*/
export const UrlCanParse = (url: string) => {
try {
new URL(url);
return true;
} catch {
return false;
}
};

export const getImageAttributes = (props: {
src: string | undefined;
srcSet: string | undefined;
Expand Down Expand Up @@ -272,7 +284,11 @@ export const getImageAttributes = (props: {
src: string;
srcSet?: string;
sizes?: string;
} = { src: props.src };
} = {
src: UrlCanParse(props.src)
? props.src
: props.loader({ src: props.src, format: "raw" }),
};

if (props.srcSet != null) {
resAttrs.srcSet = props.srcSet;
Expand Down

0 comments on commit df5a8e8

Please sign in to comment.