QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.84k stars 1.31k forks source link

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

Closed alimrb closed 3 months ago

alimrb commented 4 months ago

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

yasserlens commented 4 months ago

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 commented 4 months ago

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
shairez commented 4 months ago

Thanks for the report @yasserlens !

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

Thanks!

gioboa commented 4 months ago

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

yasserlens commented 4 months ago

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:

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

yasserlens commented 4 months ago

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:

export component$(() => return () )


- The behavior is the same on Vercel, Netlify (tested them myself) and cloudflare (original report).
gioboa commented 4 months ago

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

gioboa commented 4 months ago

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",
yasserlens commented 4 months ago

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

A few notes:

gioboa commented 4 months ago

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 👍

yasserlens commented 4 months ago

Thank you for the quick resolution @gioboa!

gioboa commented 4 months ago

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

yasserlens commented 4 months ago

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

gioboa commented 4 months ago

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

alimrb commented 4 months ago

Thanks for the report @yasserlens !

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

Thanks!

It was me reported FIRST. Me! Me! 😄

gioboa commented 4 months ago

It was me reported FIRST. Me! Me! 😄

Thanks @alimrb 😉🥇

yasserlens commented 4 months ago

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!

DeVoresyah commented 4 months ago

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

yasserlens commented 4 months ago

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.

DeVoresyah commented 4 months ago

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

bodhicodes commented 4 months ago

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.

yasserlens commented 4 months ago

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.

gioboa commented 4 months ago

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",
DeVoresyah commented 4 months ago

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: bun@1.0.1, nodejs@18.17.1
-- | --
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

bodhicodes commented 4 months ago

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

DeVoresyah commented 4 months ago

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

bodhicodes commented 4 months ago

@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.

croissong commented 4 months ago

@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

DeVoresyah commented 4 months ago

@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