qwik icon indicating copy to clipboard operation
qwik copied to clipboard

[🐞] image optimizer with imagetools in cloudflare addressed wrong srcset

Open alimrb opened this issue 1 year ago • 7 comments

Which component is affected?

Qwik Runtime

Describe the bug

In cloudflare, I see in the build logs that build directory webp images are being built correct in "build/q-[hash].webp", but in browser, developer tools says srcset ist set to "/assets/[filename]-[hash].webp".

dev and preview have no problem with it.

example here (Demo app): https://qwik-app-9et.pages.dev/

image

image

Reproduction

https://qwik-app-9et.pages.dev/

Steps to reproduce

cloudflare build

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (8) x64 Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz
    Memory: 20.31 GB / 31.90 GB
  Binaries:
    Node: 20.13.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.5.2 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Chromium (126.0.2592.81)
    Internet Explorer: 11.0.22621.3527
  npmPackages:
    @builder.io/qwik: ^1.5.7 => 1.5.7
    @builder.io/qwik-city: ^1.5.7 => 1.5.7
    typescript: 5.3.3 => 5.3.3
    undici: * => 6.19.2
    vite: ^5.2.10 => 5.3.1

Additional Information

No response

alimrb avatar Jun 29 '24 20:06 alimrb

I'm facing the same issue on Netlify - so this doesn't seem to be Cloudflare specific. The issue does not appear in local dev.

This happened after I upgraded from qwik v1.3.x to 1.6.0 Nothing else changed other than the upgrade + added "fetchPriority='high'" to one of the images - but that shouldn't be it since all images are wrongly addressed.

Client-side navigation seems to reset image URLs to have the correct path.

Here's an example image tag with the wrong path (note "assets" in the path)

<img decoding="async" loading="lazy" alt="an example image." srcset="/assets/example-m-Dmbnq0dN.webp 200w, /assets/example-m-N97A-rpt.webp 400w, /assets/example-m-CkmAcJRP.webp 600w, /assets/example-m-CJIwsBYE.webp 800w, /assets/example-m-Dfse9hf3.webp 1200w" width="1200" height="1200" class="hidden lg:block object-cover w-full h-full" q:key="X5_3">

And here is the same tag when I navigate away then come back to the same page (it appears correctly - with the correct path "build" and the names of the image files obfuscated):

<img srcset="/build/q-5m56tHTT.webp 200w, /build/q-DfewPq6b.webp 400w, /build/q-pJgHCUT9.webp 600w, /build/q-iSMLAWBN.webp 800w, /build/q-37HvYX96.webp 1200w" width="1200" height="1200" decoding="async" loading="lazy" alt="an example image." class="hidden lg:block object-cover w-full h-full" q:key="X5_3">

yasserlens avatar Jul 01 '24 18:07 yasserlens

Looking at the output of npm run build I see the following warning - could this be related? I do see it in the older qwik versions though which does not have the same bug (e.g. v1.3)

The CJS build of Vite's Node API is deprecated. See https://vitejs.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.

assetFileNames isn't equal for every build.rollupOptions.output. A single pattern across all outputs is supported by Vite.
Screenshot 2024-07-01 at 11 30 49 p m

yasserlens avatar Jul 01 '24 20:07 yasserlens

Thanks for the report @yasserlens !

Can you please provide a link to the repro github repo?

Thanks!

shairez avatar Jul 02 '24 08:07 shairez

I'm looking at this issue but I can't reproduce it locally.

gioboa avatar Jul 02 '24 16:07 gioboa

Since I can only see this in prod I don't have a ready repo - but I'll try to isolate the issue in sample code and publish it somewhere.

Note that I also tried this on Vercel using the Vercel adapter - same issue. So far, this exists on:

  • Cloudflare
  • Netlify
  • Vercel

Only in production. Local dev doesn't show it. Local prod via npm run preview does not show the issue either.

yasserlens avatar Jul 03 '24 01:07 yasserlens

I created a public repo for you to see this - see demo below: https://qwik-1-6-test-5xqrst5a4-yasserlens-projects.vercel.app/

And the code is below: https://github.com/yasserlens/qwik-1.6-test-app

Notes:

  • The issue is specifically for images imported as follows:
import SampleImage from '../media/sample.webp?jsx'

export component$(() => 
  return (<SampleImage />)
)
  • The behavior is the same on Vercel, Netlify (tested them myself) and cloudflare (original report).

yasserlens avatar Jul 03 '24 15:07 yasserlens

I see thanks, I reproduced it locally and I found the commit that generated the issue. I'm working to fix this.

gioboa avatar Jul 03 '24 15:07 gioboa

Can you try these versions to confirm the fix pls?

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@3aae568",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@3aae568",

gioboa avatar Jul 04 '24 17:07 gioboa

Thanks @gioboa - this worked when tested on a Vercel deployment.

A few notes:

  • I got a few warnings when re-building the app (https://github.com/yasserlens/qwik-1.6-test-app). These may have already been there; but wanted to highlight them in case new packages were introduced for the fix.
npm WARN deprecated @npmcli/[email protected]: This functionality has been moved to @npmcli/fs
npm WARN deprecated [email protected]: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful.
npm WARN deprecated [email protected]: This package is no longer supported.
npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/config-array instead
npm WARN deprecated [email protected]: Rimraf versions prior to v4 are no longer supported
npm WARN deprecated [email protected]: This package is no longer supported.
npm WARN deprecated [email protected]: Glob versions prior to v9 are no longer supported
npm WARN deprecated @humanwhocodes/[email protected]: Use @eslint/object-schema instead
npm WARN deprecated [email protected]: This package is no longer supported.
npm WARN deprecated [email protected]: The library contains critical security issues and should not be used for production! The maintenance of the project has been discontinued. Consider migrating your code to isolated-vm.
  • Not sure if this is normal - the package.json "build" script was reset when i installed these packages. Had to re-run the vercel adapter script to re-install things the way they were. This could be expected/normal, if not, it's worth checking.

yasserlens avatar Jul 05 '24 12:07 yasserlens

  • I got a few warnings when re-building the app (https://github.com/yasserlens/qwik-1.6-test-app). These may have already been there; but wanted to highlight them in case new packages were introduced for the fix.

I didn't touch that part

  • Not sure if this is normal - the package.json "build" script was reset when i installed these packages. Had to re-run the vercel adapter script to re-install things the way they were. This could be expected/normal, if not, it's worth checking.

That's so strange


Thanks for your feedback, I will merge the PR to solve this issue 👍

gioboa avatar Jul 05 '24 12:07 gioboa

Thank you for the quick resolution @gioboa!

yasserlens avatar Jul 05 '24 23:07 yasserlens

instead I have the feeling that it took me a long time to find the cause 🤣

gioboa avatar Jul 06 '24 07:07 gioboa

@gioboa any idea when this will hit a stable release? was hoping it would be out today with the 1.7.0 release :) Thanks

yasserlens avatar Jul 09 '24 21:07 yasserlens

We need to merge this PR https://github.com/QwikDev/qwik/pull/6643 first. There are few concerns on the resolution.

gioboa avatar Jul 10 '24 06:07 gioboa

Thanks for the report @yasserlens !

Can you please provide a link to the repro github repo?

Thanks!

It was me reported FIRST. Me! Me! 😄

alimrb avatar Jul 10 '24 08:07 alimrb

It was me reported FIRST. Me! Me! 😄

Thanks @alimrb 😉🥇

gioboa avatar Jul 10 '24 09:07 gioboa

Is there a recommended workaround until this is resolved? I have lots of images imported the same way - I'm trying to avoid refactoring my codebase to use a different approach but I'll do that if I need to. Thanks!

yasserlens avatar Jul 12 '24 23:07 yasserlens

Can you try these versions to confirm the fix pls?

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@3aae568",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@3aae568",

is that okay if I use this for temporary solution? need to deploy on production

DeVoresyah avatar Jul 15 '24 17:07 DeVoresyah

Looking at the source - this looks OK as a temp fix but may cause issues that are hard to debug later (for Qwik's core team).
I'd use it and put reminders to change this as soon as the issue is properly fixed.

yasserlens avatar Jul 15 '24 17:07 yasserlens

hi @yasserlens do you know how to use this?

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@3aae568",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@3aae568",

I'm trying to use it using bun add -D followed by the link, but getting error lockfile in cloudflare build

DeVoresyah avatar Jul 16 '24 03:07 DeVoresyah

I'm looking at this issue but I can't reproduce it locally.

Appears to be happening only in production. Works without issues on local dev and build preview.

bodhicodes avatar Jul 16 '24 05:07 bodhicodes

I'm looking at this issue but I can't reproduce it locally.

Appears to be happening only in production. Works without issues on local dev and build preview.

Yeah - that's already established with proof - see my comments above on how this can be reproduced.

yasserlens avatar Jul 16 '24 12:07 yasserlens

We are looking into it, see the progress here

In the meantime you can use this updated release

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@18b004d",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@18b004d",

gioboa avatar Jul 16 '24 12:07 gioboa

We are looking into it, see the progress here

In the meantime you can use this updated release

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@18b004d",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@18b004d",

hi @gioboa , I'm trying this one but got an error from cloudflare when deploying:

20:05:30.746 | Detected the following tools from environment: [email protected], [email protected]
-- | --
20:05:30.747 | Installing project dependencies: bun install --frozen-lockfile
20:05:30.986 | bun install v1.0.1 (31aec4eb)
20:05:30.988 | error: lockfile had changes, but lockfile is frozen
20:05:30.992 | Error: Exit with error code: 1
20:05:30.993 | at ChildProcess.<anonymous> (/snapshot/dist/run-build.js)
20:05:30.993 | at Object.onceWrapper (node:events:652:26)
20:05:30.993 | at ChildProcess.emit (node:events:537:28)
20:05:30.993 | at ChildProcess._handle.onexit (node:internal/child_process:291:12)
20:05:31.000 | Failed: build command exited with code: 1
20:05:32.305 | Failed: error occurred while running build command

already clean all the node_modules and update the lockfile by doing rm -rf node_modules bun.lockb; bun install

DeVoresyah avatar Jul 16 '24 13:07 DeVoresyah

We are looking into it, see the progress here

In the meantime you can use this updated release

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@18b004d",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@18b004d",

Thanks for the quick action on this! The workaround works well. Just a heads up: implementing this fix can cause dependency issues with some integrations—@qwikest/icons in my case.

To resolve, need to use the --legacy-peer-deps or --force install flags. In Vercel for example, in the project settings under the Vite install command override settings:

npm install --force

bodhicodes avatar Jul 16 '24 14:07 bodhicodes

We are looking into it, see the progress here In the meantime you can use this updated release

"@builder.io/qwik": "https://pkg.pr.new/@builder.io/qwik@18b004d",
"@builder.io/qwik-city": "https://pkg.pr.new/@builder.io/qwik-city@18b004d",

Thanks for the quick action on this! The workaround works well. Just a heads up: implementing this fix can cause dependency issues with some integrations—@qwikest/icons in my case.

To resolve, need to use the --legacy-peer-deps or --force install flags. In Vercel for example, in the project settings under the Vite install command override settings:

npm install --force

can't apply this changes on cloudflare

DeVoresyah avatar Jul 16 '24 14:07 DeVoresyah

@DeVoresyah correct, wasn't referencing Cloudflare, just a general heads up. I am not that well versed on Cloudflare but I think the equivalent for modifying build commands is through the wrangler.toml file. Details here.

bodhicodes avatar Jul 16 '24 15:07 bodhicodes

@DeVoresyah I solved the error: lockfile had changes, but lockfile is frozen error by bumping the bun version used by cloudflare via env variable (set in the cloudflare build settings): BUN_VERSION=1.1.14

croissong avatar Jul 16 '24 20:07 croissong

@DeVoresyah I solved the error: lockfile had changes, but lockfile is frozen error by bumping the bun version used by cloudflare via env variable (set in the cloudflare build settings): BUN_VERSION=1.1.14

thanks, you saved my life. I'm chaing the bun version to be same like in my local

DeVoresyah avatar Jul 17 '24 03:07 DeVoresyah