Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
alimrb opened this issue Jun 29, 2024 · 29 comments · Fixed by #6723
Closed

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

alimrb opened this issue Jun 29, 2024 · 29 comments · Fixed by #6723
Assignees
Labels
STATUS-2: team is working on this Scheduled for work by the core team TYPE: bug Something isn't working

Comments

@alimrb
Copy link

alimrb commented Jun 29, 2024

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 alimrb added STATUS-1: needs triage New issue which needs to be triaged TYPE: bug Something isn't working labels Jun 29, 2024
@yasserlens
Copy link

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
Copy link

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 shairez added P3: important STATUS-2: team is working on this Scheduled for work by the core team and removed STATUS-1: needs triage New issue which needs to be triaged labels Jul 2, 2024
@shairez
Copy link
Contributor

shairez commented Jul 2, 2024

Thanks for the report @yasserlens !

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

Thanks!

@gioboa
Copy link
Member

gioboa commented Jul 2, 2024

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

@yasserlens
Copy link

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
Copy link

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

@gioboa
Copy link
Member

gioboa commented Jul 3, 2024

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

gioboa added a commit to gioboa/qwik that referenced this issue Jul 4, 2024
gioboa added a commit to gioboa/qwik that referenced this issue Jul 4, 2024
gioboa added a commit to gioboa/qwik that referenced this issue Jul 4, 2024
@gioboa
Copy link
Member

gioboa commented Jul 4, 2024

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
Copy link

yasserlens commented Jul 5, 2024

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

A few notes:

npm WARN deprecated @npmcli/move-file@1.1.2: This functionality has been moved to @npmcli/fs
npm WARN deprecated inflight@1.0.6: 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 npmlog@5.0.1: This package is no longer supported.
npm WARN deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm WARN deprecated rimraf@3.0.2: Rimraf versions prior to v4 are no longer supported
npm WARN deprecated are-we-there-yet@2.0.0: This package is no longer supported.
npm WARN deprecated glob@7.2.3: Glob versions prior to v9 are no longer supported
npm WARN deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm WARN deprecated gauge@3.0.2: This package is no longer supported.
npm WARN deprecated vm2@3.9.19: 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.

@gioboa
Copy link
Member

gioboa commented Jul 5, 2024

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
Copy link

Thank you for the quick resolution @gioboa!

@gioboa
Copy link
Member

gioboa commented Jul 6, 2024

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

@yasserlens
Copy link

@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
Copy link
Member

gioboa commented Jul 10, 2024

We need to merge this PR #6643 first. There are few concerns on the resolution.

@alimrb
Copy link
Author

alimrb commented Jul 10, 2024

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
Copy link
Member

gioboa commented Jul 10, 2024

It was me reported FIRST. Me! Me! 😄

Thanks @alimrb 😉🥇

@yasserlens
Copy link

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Member

gioboa commented Jul 16, 2024

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@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
Copy link

@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
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
STATUS-2: team is working on this Scheduled for work by the core team TYPE: bug Something isn't working
Projects
Archived in project
9 participants