Editorial: refactor link headers to be declared per-type

[noamr]

• At least two implementers are interested (and none opposed):
  • N/A: editorial
• Tests are written and can be reviewed and commented upon at:
  • N/A: editorial
• Implementation bugs are filed:
  • N/A: editorial

(See WHATWG Working Mode: Changes for more details.)

---

/browsing-the-web.html ( diff )
/images.html ( diff )
/index.html ( diff )
/links.html ( diff )
/semantics.html ( diff )

[domenic]

I started doing more tactical review but then realized we should probably go further and change up our strategy. This seems directionally right (more polymorphism/abstraction) but I think we need a bit more of a holistic revamp instead of just adding abstractions over time.

Thoughts very welcome.

[domenic]

"should" seems inappropriate here. I think just removing the word or replacing it with "will" is better.

[domenic]

Actually, maybe I'm just confused. Now each link type can have up to three algorithms?

• "process a link from options"
• "process the linked resource from options"
• "fetch and process the linked resource"

(also there's "appropriate times"...)

This seems confusing, partially because the names, but partially because the setup.

Can we think harder about a clean setup here? It might expand this PR a good bit but I think it's worthwhile before we go too far.

One idea, not sure if it works:

• Every link type can define:
  • "Appropriate times to process the link element"
  • "Process the link element steps". This takes a link processing options.
  • "Process the link header steps". This takes a link processing options.
• We have a section of helpers. This could include things like:
  • "default process the link element steps" and maybe "default process the link header steps". (No longer sure if this is a good idea... who is using the default directly, these days?)
  • "create a link element request"
  • ...more...
  • Importantly, none of these are actually hooked up by default, like the current "default fetch and process the linked resource" steps are.
• We define somewhere that for every link element, when "the appropriate times to process the link element" come to pass, we 1) gather link processing options; 2) pass them to "process the link header steps" for each rel="" token.
• Similarly, we change the "process link headers" algorithm to dispatch to the appropriate "process the link header steps" for each rel="", after collecting link processing options.
• Now, we go through every link relation type and define its "appropriate times", "process the link element", "process the link header".
  • These definitions can often be "do nothing", e.g. "process the link header" for stylesheet.
  • These definitions can use shared helpers from the section mentioned above.
  • These definitions can define type-specific helpers (e.g. "preconnect") and share them between "process the link element" and "process the link header".

[noamr]

Yea I was thinking that we can go even further - to add an optional link element to link processing options, and to have one processing function per link instead of 2/3. If that processing function doesn't do anything special with pre-document options ("early hints"), it simply waits until either the element is resolved, or the document is resolved (and then creates a dummy element).

This would make link header / early hints work by default for any type of link, with special early processing when we see fit. Of course we can still limit it as an application decision.

[noamr]

@domenic I did a big refactor, I think it's much cleaner now. WATTSI is down though so no previews yet :)

[domenic]

Hmm, I'm unsure if the refactor is complete or what the intention is. I see now preload + prefetch both have "fetch and process the linked resource" and "process a link from options", but the former calls the latter which is a bit confusing. And the "default fetch and process the linked resource" still exists. And "process link headers" is no longer rel-restricted; it just always calls "process a link from options" which will crash if rel is not preload or preconnect?

What did you think about my above sketch? Maybe we should work together on a proposal for what the whole spec should look like before doing another draft?

[noamr]

Hmm, I'm unsure if the refactor is complete or what the intention is. I see now preload + prefetch both have "fetch and process the linked resource" and "process a link from options", but the former calls the latter which is a bit confusing.

Yes, the idea is that the options are created from either the link or the header.

And the "default fetch and process the linked resource" still exists. And "process link headers" is no longer rel
restricted; it just always calls "process a link from options" which will crash if rel is not preload or preconnect?

"process a link from options" is only called if it's defined for that rel (I'll double check that I didn't delete that part by mistake)

What did you think about my above sketch? Maybe we should work together on a proposal for what the whole spec should look like before doing another draft?

The PR is loosely based on it but I'll reply to it to make things clearer. But I accept the offer to work on it together rather than shoot more PRs. :)

[noamr]

Actually, maybe I'm just confused. Now each link type can have up to three algorithms?

• "process a link from options"
• "process the linked resource from options"
• "fetch and process the linked resource"

The first two were supposed to be the same one, there was a leftover...

(also there's "appropriate times"...)

That didn't really change though

This seems confusing, partially because the names, but partially because the setup.

Can we think harder about a clean setup here? It might expand this PR a good bit but I think it's worthwhile before we go too far.

One idea, not sure if it works:

• Every link type can define:

  • "Appropriate times to process the link element"
  • "Process the link element steps". This takes a link processing options.
  • "Process the link header steps". This takes a link processing options.

• We have a section of helpers. This could include things like:

  • "default process the link element steps" and maybe "default process the link header steps". (No longer sure if this is a good idea... who is using the default directly, these days?)
  • "create a link element request"
  • ...more...
  • Importantly, none of these are actually hooked up by default, like the current "default fetch and process the linked resource" steps are.

• We define somewhere that for every link element, when "the appropriate times to process the link element" come to pass, we 1) gather link processing options; 2) pass them to "process the link header steps" for each rel="" token.

You mean, to refactor all the link types that don't have headers? I think that would be great but it's a lot of work and the main benefit of this refactor is to align link headers/elements, which are currently only used by preload/preconnect (and later modulepreload). Is it worth it to refactor all the links?

• Similarly, we change the "process link headers" algorithm to dispatch to the appropriate "process the link header steps" for each rel="", after collecting link processing options.

• Now, we go through every link relation type and define its "appropriate times", "process the link element", "process the link header".

  • These definitions can often be "do nothing", e.g. "process the link header" for stylesheet.
  • These definitions can use shared helpers from the section mentioned above.
  • These definitions can define type-specific helpers (e.g. "preconnect") and share them between "process the link element" and "process the link header".

I went here in a direction where both header/element create "options" and process that... it maps to implementations pretty nicely and does not require additional helpers. But I'm also Ok with this proposal of having the helpers named after the action they perform, I can see how it might be more readable then creating those options structs.

[domenic]

You mean, to refactor all the link types that don't have headers? I think that would be great but it's a lot of work and the main benefit of this refactor is to align link headers/elements, which are currently only used by preload/preconnect (and later modulepreload). Is it worth it to refactor all the links?

That was what I'm suggesting, mainly because I think it's just too confusing what supports headers/early hints and what doesn't, otherwise. It was relatively clear before this PR when early hints/link headers were centralized; each algorithm said "bail out if not rel=X or Y". But if we introduce polymorphism then it's much less clear.

I went here in a direction where both header/element create "options" and process that... it maps to implementations pretty nicely and does not require additional helpers. But I'm also Ok with this proposal of having the helpers named after the action they perform, I can see how it might be more readable then creating those options structs.

I like the options structs! I just think we need clearly-named algorithms, or some other indication of what is supported by each rel. (E.g., maybe "process the options" + "supported link modes: early hints/headers/element".)

[noamr]

@domenic: Refactored again based on our conversations (above and on Matrix).

[domenic]

OK, I like this. It doesn't go quite as far as I was suggesting (which also involved revamping the non-header parts of links to use a similar structure, e.g. based on the options and getting rid of the defaults-and-overrides structure). But that's fine for now.

[domenic]

I pushed a commit with editorial fixups.

I remain concerned about the increased reliance on spinning the event loop. In particular, when processing early hints, I don't think there is any event loop; the Window hasn't been created yet, and as such neither has the agent and its event loop. So the definition of "spin the event loop", which relies on being called from the main thread (i.e. in a task), doesn't seem applicable.

[noamr]

I pushed a commit with editorial fixups.

I remain concerned about the increased reliance on spinning the event loop. In particular, when processing early hints, I don't think there is any event loop; the Window hasn't been created yet, and as such neither has the agent and its event loop. So the definition of "spin the event loop", which relies on being called from the main thread (i.e. in a task), doesn't seem applicable.

Good point, I'll bring back the callback instead. I wish there was a way to write code that feels more like promises, having callback everywhere feels like an inflation of indentation sometimes.

[domenic]

Focusing on trying to get the options structure right...

[noamr]

Focusing on trying to get the options structure right...

Fixed, I hope :)

[noamr]

Anything left @domenic? Thanks!

[domenic]

Sorry about the conflict with #7935, but that might help simplify this a bit, as I think now "preload" doesn't need a return value.

[noamr]

Sorry about the conflict with #7935, but that might help simplify this a bit, as I think now "preload" doesn't need a return value.

Now worries, I initiated the removing of blocking in the first place so no apologies needed :)

[domenic]

Also needs a rebase because of the integrity checks that just merged :)

[noamr]

Also needs a rebase because of the integrity checks that just merged :)

Rebased and fixed remaining stuff.

[domenic]

Down to only a few minor questions left. Sorry for the larger delay; as sometimes happens with large reviews, it gets scarier in my head the longer I delay... but I think this should be the last round.

[domenic]

So if I am reading this correctly, "process link headers" accepts empty string/no-srcset links (by parsing the empty string relative to the base URL), but "process early hints" rejects them. Is that right?

Also, "return" seems unlikely to be correct here; you probably want "continue"?

[noamr]

I'll remove this line, and each rel will deal with this individually. Only preload accepts source-sets.

[domenic]

Can you add tests for the empty string behavior? (Does not block merging this.)

[noamr]

Down to only a few minor questions left. Sorry for the larger delay; as sometimes happens with large reviews, it gets scarier in my head the longer I delay... but I think this should be the last round.

Rebased and clarified/fixed remaining bits. I hope we're near completion :)