adapter-static: Placing media before srcset in a <source> tag will prevent srcset from being crawled

[JackPriceBurns]

Describe the bug

If the attributes in a <source> tag appear like so <source media="(max-width: 400px)" srcset="/myimage.jpg"> the srcset won't be crawled and the image won't be downloaded.

When arranged like this <source srcset="/myimage.jpg" media="(max-width: 400px)"> it works as expected.

Reproduction

Add a <source> tag as mentioned above and run a SvelteKit build with adapter-static configured. Images will not be downloaded as expected.

Logs

No response

System Info

System:
    OS: Linux 5.10 Alpine Linux
    CPU: (6) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 4.01 GB / 7.77 GB
    Container: Yes
    Shell: 1.31.1 - /bin/ash
  Binaries:
    Node: 14.18.1 - /usr/local/bin/node
    Yarn: 1.22.15 - /usr/local/bin/yarn
    npm: 6.14.15 - /usr/local/bin/npm
  npmPackages:
    @sveltejs/adapter-static: ^1.0.0-next.21 => 1.0.0-next.21
    @sveltejs/kit: next => 1.0.0-next.183
    svelte: ^3.43.1 => 3.43.2

Severity

serious, but I can work around it

Additional Information

No response

[rmunn]

The way the regex is written in

kit/packages/kit/src/core/adapt/prerender.js

Line 43 in e664d90

function get_srcset_urls(attrs) {

will only match if the srcset attribute is first. I don't have time to build a better regex right now, but that's definitely the cause of this behavior.

[Conduitry]

I've wondered whether it would be a good idea to give up on regexes here and just use something like parse5 to parse the HTML and walk the AST ourselves to find stuff more robustly. If this only ever happens at build time, I guess I see less of a cost to doing this with a bigger slower library.

[helmturner]

If this only ever happens at build time, I guess I see less of a cost to doing this with a bigger slower library.

Considering projects such as Elderjs, Nextjs (with hybrid mode), etc, and considering the (implied?) objective of making Kit a write-once, deploy however framework, I would call back to one of Rich's arguments about the perils of "convenient escape hatches".

I've got a fair bit of regex deep-dive in me. Per the discord discussions about compositional regex, lookaround assertions, etc, I'd like to take a swing at a full refactor of the regex in prerender.js to address all related issues (there seem to be many).

Would it be possible to get a clearer definition of the objectives by aggregating the related issues somehow? It may also help to review/merge from PRs any valid tests related to regex in this file. Would help me during the refactoring process to validate as I go.

Of course, maintainability will remain a top concern - but I don't think it has to be sacrificed here in favor of a dependency-free solution. I can open a new issue for all prerender.js regex, if you want to link it all under that? Lmk.

[si3nloong]

@Conduitry I prefer to use parse5 way instead of the regexes.

[rmunn]

I've wondered whether it would be a good idea to give up on regexes here and just use something like parse5 to parse the HTML and walk the AST ourselves to find stuff more robustly. If this only ever happens at build time, I guess I see less of a cost to doing this with a bigger slower library.

I think I'm in favor of a proper parser instead of regexes, too. Two recent Svelte bug reports —sveltejs/svelte#6844 and sveltejs/svelte#6923 — were both caused (I'm 95% certain) by an imprecision in the regexes Svelte uses to detect opening & closing <script> or <style> tags. A proper parser wouldn't suddenly produce incorrect results because a comment was added or because attributes were reordered.

[Rich-Harris]

fixed by #3288