fix: check srcset when hydrating to prevent needless requests

[dummdidumm]

fixes #8838

@benmccann could you test if this fixes the issue?

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[benmccann]

Amazing! Thank you so much for working on this!!

I'm afraid it's not quite working as it's not taking into account the possible width or pixel density descriptor: https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/srcset#value

[dummdidumm]

What do you mean by that exactly? In which case would it result in a false negative or false positive? You're right that if the widths are different it's technically wrong and we should account for the "url is the same but width descriptor is different" case - is that what you mean?

[benmccann]

What do you mean by that exactly? In which case would it result in a false negative or false positive?

It seems my explanation was incorrect. I noticed that we weren't checking for the descriptors and jumped to the wrong conclusion and mis-interpretted the debugger output.

You're right that if the widths are different it's technically wrong and we should account for the "url is the same but width descriptor is different" case - is that what you mean?

It wasn't, but we probably should account for that as you say.

What's really happening

It seems that the existing src_url_equal method isn't working. I'm not sure if this is also an issue with img src in addition to srcset as I haven't tested that, but I would assume so. Perhaps we were relying on an undocumented feature of Chrome that's changed?

Here's a screenshot showing that the temporary element href is still the full URL:

Here's a screenshot showing false being returned:

[dummdidumm]

I don't understand - the idea is that assigning to the href of the hidden a that any relative URLs are resolved to absolute URLs, so that they can be compared.
Does that mean that in the SSRd output the URL is relative and not absolute? So that the browser html contains <a href="_assets/.." but the comparator function expects that it's <a href="<host>/_assets/.. so it fails? In which case, we would need an additional "are the urls the same without creating a hidden href".

[benmccann]

the idea is that assigning to the href of the hidden a that any relative URLs are resolved to absolute URLs, so that they can be compared

Ah, you might be right. I was thinking it was the reverse (convert them to relative) and maybe Chrome changed or something.

So then I guess src_url_equal intends for the first parameter to be the img.src which the browser has converted to a full URL and it appears that the browser doesn't convert the URL to a full URL for srcset. But then in that case the thing I'm confused about is how the second parameter to srcset_url_equal is ending up as a full URL with origin. Shouldn't it just be the URL as I've provided it, which starts with /? If it were, then we could just do === and skip all this complication.

Does that mean that in the SSRd output the URL is relative and not absolute?

Yes. It looks as below (also visible on https://c3.ventures/)

<picture><source srcset="/_app/immutable/assets/living-carbon-logo.92781a1d.avif 530w" type="image/avif"><source srcset="/_app/immutable/assets/living-carbon-logo.181689e7.webp 530w" type="image/webp"> <img src="/_app/immutable/assets/living-carbon-logo.dae03d6f.png" width="530" height="89" alt="Living Carbon"></picture>

[dummdidumm]

Ok now I'm confused aswell - how is the second parameter becoming a full URL but the first is not?

[gtm-nayan]

reading img.src returns the resolved URL, the second parameter has full URLs on the client-side because of how vite and vitest output code for SSR IIRC, it's just string URLs on the server output but on the client output it's emitted as new URL(..., location.href).href + ""

[dummdidumm]

Yeah I just noticed that, too - either that's Vite or it's vite-imagetools, but regardless of who it is, that's why it happens. So we need to account for the things being the other way around, too.

[dummdidumm]

Ok @benmccann I think this is ready for another test on your site - according to my tests on svelte.dev this should work as expected now. I also added some explanation and a test.

[benmccann]

I wonder if perhaps we should see whether it'd be possible to fix Vite to output the same in SSR and on the client. That would remove the need for this PR I think reducing complication in the codebase and perform better

[benmccann]

For reference, here's the generated code:

const sources$m = {
  avif: [
    {
      src: "" + new URL("../assets/living-carbon-logo.92781a1d.avif", import.meta.url).href,
      w: 530
    }
  ],
  webp: [
    {
      src: "" + new URL("../assets/living-carbon-logo.181689e7.webp", import.meta.url).href,
      w: 530
    }
  ]
};

I believe vite-imagetools just outputs a placeholder, which gets replaced by Vite. I'm guessing it's ultimately coming from Vite's relative path support.

In SvelteKit, if I change:

base: ssr ? assets_base(kit) : './',

To:

base: ssr ? assets_base(kit) : '/',

Then I get as output:

const sources$o = {
  avif: [
    {
      src: "/_app/immutable/assets/c3-logo-alpha.eabb7fc9.avif",
      w: 262
    }
  ],
  webp: [
    {
      src: "/_app/immutable/assets/c3-logo-alpha.77dad92c.webp",
      w: 262
    }
  ]
};

[benmccann]

we've been using @ts-ignore as a TODO and @ts-expect-error if it's something that we intend to leave like that permanently. it could probably use a comment next to it either way

it's a shame you'd have to do this though. Vitest doesn't show it in their example: https://vitest.dev/config/#environment. I wonder if this is something worth bringing up with them or something that we can fix with a global TS declaration

[dummdidumm]

Not sure why I needed this, I don't see the error anymore - removed it

[benmccann]

I just tested this PR and it's fixed the issue for me. It seems to be running about 5-6x faster now!

I sent sveltejs/kit#10287 to also improve things on the SvelteKit side