fix: construct query params from location.search on rehydrate

[pzerelles]

Fixes #669

When rehydrating a page, the query params were set to URLSearchParams(s(page.query.toString())). This change uses location.search when page.query is empty.

[benmccann]

This isn't a section of code I'm super familiar with. Can you explain under what scenarios page.query.toString() would be different from location.search?

[pzerelles]

Actually I just didn't want to prevent what was happening before if page.query is set. We could probably just use location.search. Maybe the original author can tell us if it is really necessary.

[benmccann]

Yes, this seems simpler. Still, I'm wondering what exactly the bug was. E.g. there are other places we use page and I wouldn't want there to be issues there as well. Can you explain why or when page.query.toString() is different from location.search?

[pzerelles]

The bug was that when rehydrating pages on static sites, the query string from the url was not used, only the query prop of the page object which was empty.

[benmccann]

the query prop of the page object which was empty

That's what concerns me about this fix. It seems like a bit of a workaround. Why is the query prop empty? It seems like the query prop is still going to be broken for anyone who tries to use it

This is only going to work on the browser-side on rehydration, so it seems like it could catch people off-guard that the server-side isn't rendered as they expect

[benmccann]

I added an explanation for what's happening on the ticket: #669 (comment). I'm not sure what the other maintainers think, but at the moment I'd lean towards disallowing usage of $page.query on prerendered pages instead

[johnnysprinkles]

I'd be curious if @benmccann or @pz-mxu has anything thoughts about this RFC I submittted today, making the argument that this PR is a special case of a general issue. sveltejs/rfcs#52

[Rich-Harris]

Using page.query is deliberate — the idea is to guarantee consistency between server and client renders. Breaking that guarantee feels like a dangerous road to go down. If you don't know location.search at SSR time (because you're prerendering) then it feels more appropriate to disable SSR for that page than to render an incorrect page, surely?

[benmccann]

Thanks for taking the time to investigate and propose this fix. I'm going to go ahead and close this given that Rich has the same opinion as I do that this isn't the correct way to address the issue

[johnnysprinkles]

So this issue seems to be fixed now... if I serve a staticly built page and put this code in there:

	if (!prerendering) {
		console.log($page.url.searchParams.get('foo'));
	}

I get the actual runtime per-request value of "foo" as seen by the client, instead of the value as was snapshotted at prerender time. I'd like to find out which commit change all this around!