Use raw attribute href on hx-boost

[alexpetros]

Description

This resolves a bug with file system URLs on Windows. It reverts part of a previous change (#1338) where URLs were normalized prior to the request in order to reduce key collisions in the history cache.

The gist is that on Windows, elt.href normalizes relative URLs like this:

Boost HTMLAnchorElement
        href resolves to file:///E:/test
        raw attribute : /test
should swap ? false
Response Status Error Code 404 from file:///E:/test

Why does elt.href normalize relative URLs on Windows file systems differently from how anchor elements normalize URLs? It doesn't. It normalizes the URL into something that is wrong (on Windows) before the mockServer() function can get to it. It turns this:

<a href="/profile">My Profile</a>
<a hx-boost="/profile">My Profile</a>

into this:

<a hx-boost="https://example.com/some/qualified/url/profile">My Profile</a>

Which, while usually the same thing, is still an additional normalization that we perform with JavaScript which deviates from the browser's request normalization, and I can't see any justification for keeping it. Because htmx's history cache is based entirely on response URLs, not request URLs, I am reasonably confident that reverting this line doesn't affect the optimization from #1338 at all.

And, conceptually, it makes a lot more sense that we would just pass the raw attribute value to the browser's request engine and let it do whatever normalizing it does there—which is less likely to deviate from regular <a href=""> behavior. Doing it twice introduces the possibility of error without any clear benefit.

@Telroshan and I discovered this bug as part of #1732. He doesn't totally agree with me that this change is necessary so if you want a competing view go read that thread. See below for some of that discussion, but we agree now.

Testing

Passes locally and on the CI. I also ran a Windows Server GitHub runner and while that was not very reliable at running our tests, it didn't have the same issue anymore at least.

This change should make it possible for Windows users to run our headless test suite with no errors using npm t.

@Telroshan tested on windows and this fixes the tests so he can proceed with #1687

[Telroshan]

I'm ok with the change, I'm not 100% confident that it's not going to cause some edge case issues, but I guess we'll quickly find out if someone comes pick a fight with us on this when the update rolls in 😆

Why does elt.href normalize relative URLs on Windows file systems differently from how anchor elements normalize URLs? I'm honestly not sure. However, in effect that line of code turns this:

Just to make sure here: anchors behave the same, if you open a raw HTML file (i.e not HTTP served) on Windows, and hover the following;

<a href="/test" id="test">Click me</a>

This is what I get when hovering the link in the browser:

And accessing href in the console:

test.href
> 'file:///E:/test'

So the URLs are the same, elt.href and anchors resolve to the same URL.
It does so on Windows because file paths are different from Linux/Mac, which if I'm not wrong, look like /var, /etc and so on (while on Windows, you always prefix with the disk name, thus the /E:// path, and the browser adds that file:// prefix as it's not HTTP)

As such, just to make it clear again:

This resolves a bug with file system URLs on Windows.

A bug with the way the tests are run, because they are run from a file (HTML file opened directly, instead of being served through HTTP) but we are mocking URLs as if we were on a HTTP-served page, thus the error on Windows (that doesn't happen on linux & max as relative HTTP URLs look the same as disk URLs)
Otherwise it works just fine on Windows, you basically get what you're asking for in the environment you're in

[alexpetros]

So the URLs are the same, elt.href and anchors resolve to the same URL.

Oh okay, I know what's going on here, it's about the mocks. We normalize the URL before the mock server gets to it. They do in fact normalize the same way. Got it, got it. I updated the description.

A bug with the way the tests are run, because they are run from a file

🤷 The tests should be runnable from a file! I take your point that we should probably also set up some tests that test from a server, but it's a good thing that the tests should work in as many environments as possible. If anything it's a bug with the tests themselves, but if you look at this commit in isolation: that line of code introduces unnecessary surface area for inconsistencies; I know why this fence was erected and would like to tear it down.

[Telroshan]

I'm ok with the change, I mean as long as it works, I'm totally fine with it

I agree with your argument that if the browser already handles it, we shouldn't add a useless instruction

Now, considering all is good and tests work, conceptually I would still advocate for running the tests in a http served environment rather than a file environment
If you allow me this analogy, I see it like running say Windows tests on a unix system, in which I wouldn't be surprised to get errors in the future that are relative to the environment in which tests run (here file:// protocol in reality vs http:// that is mocked and tested)

But again, that's just a side conceptual debate here ; I'm not saying this PR should not be merged, I'm all ok with it. Again, as long as it works, it's perfectly fine for me

[alexpetros]

For sure, I'm actually going to look into multi-platform tests after this.

[croxton]

If we don't retrieve the fully resolved url of a relative href then it's possible to cache two pages that share the same url slug with the same key, which can lead to the wrong page being retrieved from the cache on history restore.

For example:

1. A page at the document root /foo.html has a boosted anchor with an href profile.html which loads a page at /profile.html. When the page is cached to history it has the cache key profile.html.

2. A page at /bar/baz.html has a boosted anchor with an href profile.html which loads a page at /bar/profile.html. When the page is cached to history it also has the cache key profile.html.

That was the reasoning behind that part of my original PR.

[alexpetros]

Right—do you have any evidence that needs to be done in the the request? Because that's the only part of your chance that this reverts. I fully understand why we're normalizing the URL before manually saving to the history API, but all of that logic happens in the response. I don't think this line in the request does anything useful.

(it might be nice to have a test case for this, the history API logic is not straightforward)

[croxton]

I tested my scenario with your change (on modern browsers) and I can't reproduce the key collision, so I think this is good to go.

I fully understand why we're normalizing the URL before manually saving to the history API, but all of that logic happens in the response

Just to be clear, the history cache in localStorage is saved in a subsequent request + response cycle, when the user navigates away from the current page, NOT as part of the response to the click on the original boosted link.

[alexpetros]

I tested my scenario with your change (on modern browsers) and I can't reproduce the key collision, so I think this is good to go.

Thank you very much @croxton! If you're able to share your testing steps I'd be curious to see them.

Just to be clear, the history cache in localStorage is saved in a subsequent request + response cycle, when the user navigates away from the current page, NOT as part of the response to the click on the original boosted link.

I'm talking about this part, which happens in handleAjaxResponse. I might be totally wrong though, or we're talking past each other.

htmx/src/htmx.js

Lines 3294 to 3307 in 05d53f4

	if (hasHeader(xhr, /HX-Location:/i)) {
	saveCurrentPageToHistory();
	var redirectPath = xhr.getResponseHeader("HX-Location");
	var swapSpec;
	if (redirectPath.indexOf("{") === 0) {
	swapSpec = parseJSON(redirectPath);
	// what's the best way to throw an error if the user didn't include this
	redirectPath = swapSpec['path'];
	delete swapSpec['path'];
	}
	ajaxHelper('GET', redirectPath, swapSpec).then(function(){
	pushUrlIntoHistory(redirectPath);
	});
	return;

[croxton]

Thank you very much @croxton! If you're able to share your testing steps I'd be curious to see them.

Just some simple manual tests - I created some html pages as per my scenario and used boosted links with hrefs containing relative links, browsed around a bit, then examined the cache entries that get created in the localStorage history cache.

I'm talking about this part, which happens in handleAjaxResponse. I might be totally wrong though, or we're talking past each other.

I've been testing for key collisions in the localStorage cache. pushUrlIntoHistory() will push a history entry to the user's browser once a request has been received - that's a separate thing to the history cache which is created later when the user navigates away from the page that had the browser history entry created for it. No chance of a collision on a push() since the browser will automatically resolve the link for the history entry. Your PR had the potential to impact on the value used for the localStorage cache key (which should correspond to the URL pushed previously) but luckily it doesn't seem to in my testing.