kit icon indicating copy to clipboard operation
kit copied to clipboard

Don't pass params from the server

Open johnnysprinkles opened this issue 2 years ago • 5 comments

I'd argue that we shouldn't pass the path params from server to client, for the same reason we're no longer passing the URL -- the real version on the client is the authoritative version.

This to me is just a logical extension of #3942.

This change would fix #1533, and obviates #1871.

The motivation for this is, in Big Tech, you often find yourself in a Java monoculture. No problem though, adapter-static to the rescue. But the issue is, in general, every website will have parameterized paths and it's a little ugly using adapter-static with parameterized routes. You can do it but you have the regex-parse the URL yourself instead of relying on the pagestore provided params. This fixes that issue.

As with my previous PR from a few days ago, not super optimistic about this going in, but useful to have for reference in case anyone wants to patch it or make a fork.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

johnnysprinkles avatar May 16 '22 02:05 johnnysprinkles

🦋 Changeset detected

Latest commit: dde30cd12acc64408ddac7586c5f1a81a1f2f510

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar May 16 '22 02:05 changeset-bot[bot]

Note that I started documenting various SvelteKit patterns, that I plan to reference in feature requests and PRs going forward. This particular PR would be in service of Pattern IV. See https://royalbarrel.com/notes/3

johnnysprinkles avatar May 31 '22 00:05 johnnysprinkles

Just rebased, and also I verified that this change covers both the page store and the params which the clientside load() call would see.

johnnysprinkles avatar Jun 07 '22 03:06 johnnysprinkles

OK, this is rebased today and ready for review. Let me know if there's a better way to parse the URL params rather than running it through navigation_intent.

johnnysprinkles avatar Nov 05 '22 22:11 johnnysprinkles

Apologies for leaving this PR out in the sun for so long — sometimes when there's enough activity on the PR page things like this sort of get trudged into the ground until you forget to pick them up.

The original reason for passing params from the server, IIRC, is admittedly somewhat esoteric: on the NYT homepage there's a covid tracker embed generated from a SvelteKit app, nestled among everything else:

image

There's a variety of embeds available; the one that gets selected is based on params. If params is derived from the URL, the app gets terribly confused and nukes the embed. Passing along the params that were actually used to render the embed (while also disabling client-side navigation) allows it to work.

There are definitely more elegant ways that problem could be solved, though they'd involve either bypassing SvelteKit or adding some sort of embedded output that renders self-contained-HTML-plus-hydration code but omits the SvelteKit runtime. That said, the current approach does have some benefit: get_navigation_intent has to do work, and even if it's negligible in the grand scheme of things, avoiding work (especially at startup) is good.

So I'd love to better understand what the motivation for this is. If you're using adapter-static and the page is prerendered, the params object should also be prerendered. When are the inline params incorrect?

Rich-Harris avatar Nov 16 '22 20:11 Rich-Harris

This is really interesting, I've never thought about rendering "embeds" just whole pages, which presumably live among the same URL structure at runtime as they do at build time. I can tell you about my use case:

We have several routes, some of which are parameterized, for example a parameterized one would be like:

routes/song/[id]/+page.svelte

Where the ID is a numeric value. This won't prerender by default so we explicitly prerender it with a placeholder value, i.e. an entry like this in the svelte config prerender entries:

'/song/id'

Which means pass the string "id" as the ID. Then we do all the value specific data fetching clientside only (either in onMount or in a load that's guarded off by env values). That's not to say we might not also do some data fetching at pre-render time, but that would only be for data that's constant at least until the next production deployment.

So now in our build folder we have a /song/id.html file (or is it /song/id/index.html, not sure, recalling this from memory). Then we just rewrite all /song/\d+ paths to serve that file. This whole setup is an alternative to using an SPA style fallback, with the advantage of each page's basic layout skeleton appears immediately before JS has hydrated.

And this all works great except that getting the "id" value has to be done manually. We have code like this that I'm again recalling from memory:

import { base } from '$app/paths';

let path = window.location.pathname;
if (base) {
  path = path.slice(base.length);
}
let m = path.match(/^\/song\/(\d+)$/);
let id = m[1];

Not too bad just adds a place we have to update if the paths ever change.

johnnysprinkles avatar Nov 26 '22 18:11 johnnysprinkles