kit icon indicating copy to clipboard operation
kit copied to clipboard

Replace `data-sveltekit-prefetch`/`prefetch(...)`/`prefetchRoutes(...)`

Open Rich-Harris opened this issue 3 years ago • 15 comments

Doing things slightly backwards here — a PR before an issue/discussion — because whether or not it's a good idea largely depends on how expensive the implementation is.

Good news: it's cheap. The changes here add 402 bytes (187 zipped) to the start bundle, which I personally think is acceptable, even if we should use that yardstick sparingly. (Will update these numbers as they change; I suspect there's room for some code golf.)


Today, any link with data-sveltekit-prefetch (on the element itself, or a parent) gets prefetched when you a) tap on it or b) rest the mouse over it. This is a useful feature that helps navigations feel fast, but I don't think the design is fully baked. I'd like to make some changes while we still can.

  • Firstly, it's too aggressive and not aggressive enough — some people want it to only be triggered on mousedown (so that data doesn't get stale in the half-second it takes to click a link, and requests aren't made that end up being unused), while others want it to happen for all links in the viewport.
  • Secondly, it works for mouse/finger-driven navigation, but not for keyboard-driven navigation. That's not great.
  • Thirdly, it ignores any preference the user might have stated to conserve data (navigator.connection.saveData, which is supported in Chrome).
  • Finally, the name is wrong. As far as HTML is concerned, prefetch is a gentle hint to the browser; preload is an instruction. In the SvelteKit context, it causes load functions to run (which may or may not involve a fetch). data-sveltekit-preload is thus a better name.

preload vs prepare

We need to distinguish between the code and the data. It's possible (and in many cases makes sense) to get the code for a given route without getting the data. Let's call these two things prepare and preload (open to bikeshedding). Someone might want to prepare all the routes that are linked to from the current page, or within the current viewport, but want to delay loading until the user actually clicks.

(They might also want to preload all those pages, but I'm not sure we should let them as it feels like a bit of a footgun. Right now, you can only preload for a specific navigation, and the promise is discarded as soon as you preload something else or navigate. We gloss over the fact that preloading makes data stale, because in practice it's fine when we're talking about a link that the mouse is already at rest over, but it would be less fine if we had the data for potentially hundreds of pages just sitting around in memory, only to discard and rebuild the cache every time we navigated.)

Personally I think I'd prefer a declarative approach to this over a programmatic one (though people can of course use the existing APIs, though more on that later). We could have options like these, in increasing order of aggressiveness:

  • tap — triggered by mousedown, touchstart, or hitting Enter over a focused link
  • hover — (current behaviour) triggered by the mouse coming to rest over a link, but also touchstart or Enter on a focused link
  • viewport — triggered when a link enters the viewport
  • page — applies to every link that exists after a navigation

So you might have something like this...

<body data-sveltekit-prepare="viewport" data-sveltekit-preload="tap">

...which would mean 'eagerly import the modules for every page I might visit next, but only load the data when I tap on the link'. viewport and page would be invalid values for data-sveltekit-preload. Since prepare is a prerequisite for preload, data-sveltekit-preload="hover" would imply data-sveltekit-prepare="hover" if it was otherwise unset (or was set to "tap", which would be nonsensical).

All of these should be disabled when navigator.connection.saveData === true, I think.

Fixing the APIs

We also have two programmatic APIs for this stuff — prefetch(href) and prefetchRoutes(hrefs). If we were to rename data-sveltekit-prefetch to data-sveltekit-preload then we should rename prefetch(href) to preload(href).

Meanwhile prefetchRoutes(hrefs) could become prepare(...hrefs). I don't know if we should keep the behaviour where if no arguments are supplied, it imports your entire app. Maybe?

Questions

  • Does this idea broadly make sense?
  • Are data-sveltekit-prepare and data-sveltekit-preload good names?
  • Are these the right declarative options? Are there others we should consider? Could there be a situation where we need some combination of options? Do we need any further configuration, like 'milliseconds the mouse needs to stay still before we call it a hover'?
  • How hairy are the implementation details going to be? Is it a terrible idea to do document.querySelector('a[data-sveltekit-prepare="viewport"], [data-sveltekit-prepare="viewport"] a') inside a scroll event listener? (Probably.)

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

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

Tests

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

Changesets

  • [ ] 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

Rich-Harris avatar Nov 23 '22 02:11 Rich-Harris

🦋 Changeset detected

Latest commit: 2553a9d4114b93c4270e3d8b7805cc8fb973a93c

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

This PR includes changesets to release 2 packages
Name Type
@sveltejs/kit Patch
create-svelte Patch

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 Nov 23 '22 02:11 changeset-bot[bot]

I can hear @tcc-sejohnson's voice telling me that we need to refactor client.js so that it isn't just one giant hairball of a closure, and the voice isn't wrong — this PR continues some bad habits. It would be nice to tidy it up soon if we can do so without increasing the bundle size

Rich-Harris avatar Nov 23 '22 02:11 Rich-Harris

glad i could wheedle my way into your subconscious

I'd be curious about the memory-mangement characteristics of a less-closure-y implementation as well. More efficient because the garbage collector knows what to do better? Less efficient because the runtime is smart? Honestly, I have no clue.

  • in general: if someone wanted to do something completely different, they still can't replicate the functionality easily. I'm wondering if making something like find_anchor public API would be good for this. Just finding an anchor isn't enough, we also take into account reload etc -> looks easy at first, but isn't that easy after all
  • the API: I like the idea of splitting it up into loading the files and loading the data, and about the defaults we present. I'm not sure about the wording. preload in the context of a browser means preloading files, not the data. prepare doesn't have any strong meaning to me, so it's a bit of a "what is this"-thing at first. I think prefetch is actually a good name because it contains fetch which I relate to data. preload would then be open for "preload the files" --> my proposal: prefetch stays as is, preload is the new thing for only loading the JS.
  • the implementation: not sure about the intersection observer because of its drawbacks if <a> tags appearing afterwards. Is querying the dom really that horrible inside scroll? If you make the event listener passive and debounce the dom traversal (~500ms) it shouldn't really affect perf. The code might become a little less, too.
  • the size: for some other features I can imagine adding the functionality lazily to client.js so it's treeshakeable. I don't see how that would be possible here, because it's declarative from the outside through data-sveltekit-X attributes, which is onfortunate.

Overall this change looks largely good to me, although I also have to admit that this feels like a "diminishing returns"-situation.

dummdidumm avatar Nov 23 '22 15:11 dummdidumm

I'm not sure about the wording

The reason for preload is that we're literally pre-running load functions (which may or may not involve a fetch) ahead of a navigation.

preload in the context of a browser means preloading files, not the data

So does prefetch :) The difference is that preload is an instruction ("you shall load these files") whereas prefetch is a hint ("we may need these files at some point; load them if you feel like it"). Of those, preload is a closer match for what we're doing here, so I think it's the better of the two.

Agree that prepare is slightly vague, I just couldn't come with anything better. Definitely open to suggestions.

Is querying the dom really that horrible inside scroll?

Yep! You have to query the DOM, and for each <a> element, a) walk up the tree to find its nearest data-sveltekit- options and b) determine whether it's in the viewport, which means forcing layout, which could easily lead to jank. The best case scenario is that we waste users' battery life; the worst is that SvelteKit apps become associated with dropped frames when scrolling. Debouncing helps, but 500ms is plenty of time for someone to click on a link that just became visible.

And it still wouldn't completely solve the problem — if an <a> appeared inside an {#if} block it wouldn't get registered if the user didn't scroll before clicking on it (unless we started using mutation observers, which is a bad idea).

For those links, failure to eagerly import their code isn't catastrophic — it just means that SvelteKit will automatically do it on hover or tap instead of when it first becomes visible. Given that those sorts of links are the minority, it feels like a lousy trade-off.

Rich-Harris avatar Nov 23 '22 18:11 Rich-Harris

When I think about the definition of the term preload I think primarily about what it does (fetch static assets for the current page before they are needed) and not about whether it's a hint or a command. If there were no existing browser feature called preload then I'd agree that it's a great name for invoking load before it's needed. However, given that there's already a widely known feature with this name, I feel quite strongly we should not overload it with another definition.

If we want to keep load in the name then I think it's be better to avoid pre and use something like earlyLoad, foreload, loadAhead, anteload, speculativeLoad, pilotLoad, advanceLoad, etc. Alternatively, we could use something other than load like preExec, preGet, predispatch, prerun, preFulfil etc.

benmccann avatar Nov 23 '22 18:11 benmccann

By running the load function we are getting the files (whether __data.json or something you manually fetch) needed to render the page. preload just means we do that before the navigation — in other words 'fetch assets before they are needed'.

Rich-Harris avatar Nov 23 '22 22:11 Rich-Harris

I just wanted to point out that it doesn't make much sense to preload on keydown, IMO. The click event is dispatched immediately after the keydown event.

PatrickG avatar Nov 24 '22 02:11 PatrickG

By running the load function we are getting the files (whether __data.json or something you manually fetch) needed to render the page. preload just means we do that before the navigation — in other words 'fetch assets before they are needed'.

I totally get the reasoning for wanting to name this "preload" because of the "load" function, but in my mind that name is just much more connected with browser things. Dare we make a poll on this?

dummdidumm avatar Nov 24 '22 08:11 dummdidumm

it doesn't make much sense to preload on keydown

Au contraire! When the click happens, you have about 100ms to update the screen if you want the transition to be perceived as instant. This REPL demonstrates that the gap between the keydown and click events is about 90ms for me on average. For some people it'll be less, for some people more, but that 90ms headstart basically doubles the window. It can be the difference between 'slick' and 'sticky'.

Though your comment did make me realise that we're responding to touchstart but not mousedown (because previously we always listened for mousemove instead, making mousedown redundant) — will fix that.

that name is just much more connected with browser things

I'm honestly confused by this. <link rel="preload"> means 'load stuff before we need it', data-sveltekit-preload means 'load stuff before we need it'. What am I missing?

Rich-Harris avatar Nov 24 '22 11:11 Rich-Harris

Au contraire! When the click happens, you have about 100ms to update the screen if you want the transition to be perceived as instant. This REPL demonstrates that the gap between the keydown and click events is about 90ms for me on average. For some people it'll be less, for some people more, but that 90ms headstart basically doubles the window. It can be the difference between 'slick' and 'sticky'.

You are using a button instead of an a element, and you check for the Space key instead of the Enter key.

Try this: https://svelte.dev/repl/25cf577d4afe47639f4114687028805c?version=3.53.1 I get a "headstart" of 0 to 1 ms :sweat_smile:

PatrickG avatar Nov 24 '22 14:11 PatrickG

I'm honestly confused by this. <link rel="preload"> means 'load stuff before we need it', data-sveltekit-preload means 'load stuff before we need it'. What am I missing?

For me it means "load files before we need them", not "call the load functions before we need the data".

  • Your proposal: data-sveltekit-preload => call load functions
  • My intuition when I see that: data-sveltekit-preload => preload files for that page

Therefore my proposal:

  • data-sveltekit-preload => preload files for that page
  • data-sveltekit-load-eagerly or sth similar => call load functions of that page

...honestly I don't have good names for either of that. Is there some way to merge these things into one? Like data-sveltekit-preload="files viewport, load tap"? Mhhm probably too clunky.

Goddamnit I might give in to prepare/preload after all... naming things sucks.

dummdidumm avatar Nov 24 '22 14:11 dummdidumm

But what is the effect of running load, if not to load (JSON) files? All we're ever doing is loading "files", whether that means importing modules, loading assets, or fetching dynamically rendered data from the server. The boundaries we draw around those things are largely meaningless

Rich-Harris avatar Nov 24 '22 15:11 Rich-Harris

I get a "headstart" of 0 to 1 ms

Well I'll be. I used a <button> to test because I'm a Firefox user, and Firefox doesn't focus links by default unlike Chrome. It wouldn't have occurred to me in a million years that buttons would click on keyup while links click on keydown. What is this ridiculous platform

Rich-Harris avatar Nov 24 '22 17:11 Rich-Harris

means 'load stuff before we need it', data-sveltekit-preload means 'load stuff before we need it'. What am I missing?

The browser's preload loads static assets for the current page whereas this functionality would largely call dynamic API endpoints for a yet-to-be-visited page. That distinction immediately jumps out at me.

The static asset vs dynamic endpoint is important because static assets go into the browsers cache and are unlikely to have new content, so you can do it pretty safely. With dynamic endpoints being fetched into the application's memory you need to think about each individual one much more closely to think about whether it's safe. E.g. if the user may edit some piece of data on the current page, then perhaps getting the data for the next page before that's done would cause stale data to be shown. Just as importantly, static assets are very cheap to serve whereas dynamic endpoints place much heavier load on the systems involved.

The difference of fetching data for the current page vs possible future page is also quite large to me. Fetching data that will be needed for the current page is almost always desirable and we can largely activate the feature in the framework. Fetching data for future pages is much more speculative and you need to think a lot more about the cost / benefit tradeoffs you will be incurring and whether that's desirable.

benmccann avatar Nov 25 '22 14:11 benmccann

if the user may edit some piece of data on the current page, then perhaps getting the data for the next page before that's done would cause stale data to be shown

To be clear, preload only happens when the user is hovering over a link or actually taps it (depending on how you've set things up). prepare can happen when links enter the viewport, or are present on the page following a navigation, but this doesn't involve calling load, only eagerly importing code.

Rich-Harris avatar Nov 26 '22 13:11 Rich-Harris

We reached a decision about naming:

  • data-sveltekit-preload-code
  • data-sveltekit-preload-data

Rich-Harris avatar Nov 26 '22 17:11 Rich-Harris

Sorry I had to jump off the call just before we got to discussing this. I think these names are pretty clear if a bit long. I think that for me the difference that sticks in my mind about preload vs prefetch is whether you're loading stuff for the current page or the next page rather than hint vs instruction, so I'll throw out data-sveltekit-prefetch-code and data-sveltekit-prefetch-data for consideration though I'm probably a bit late on the subject (and then I'd probably shorten data-sveltekit-prefetch-code to just data-sveltekit-prefetch).

benmccann avatar Nov 26 '22 22:11 benmccann

It's less about 'current page' or 'next page', more about whether we want the resources now or later. In the preload-data case the answer is 'now': preload is clearly more appropriate than prefetch.

For preload-code 'now' vs 'later' depends on whether it's tap or hover (now) or viewport or page (later). It would obviously be very confusing to have two attribute names for that, and choosing prefetch over preload would necessitate an explanation in the docs as to why we're using preload for one and prefetch for the other even though timing-wise they're often identical. (In future I could see us wrapping page and viewport preloads in scheduler.postTask(...) once it's sufficiently widely supported, but I don't think that fundamentally changes anything).

In any case: perhaps I'm just projecting my own ignorance, but I suspect the majority of web developers couldn't tell you the difference between preload and prefetch without consulting MDN. So we should probably just pick the name that makes the most sense absent any considerations around timing, and here preload is, to me, a clear winner over prefetch: load is a universal term for getting stuff that works equally well with code, data and assets (to the extent that you can draw clear boundaries around them, that is — for example 'code' will include CSS files in this context, which you'd probably normally put in the 'assets' category), whereas fetch more narrowly describes a mechanism for getting data.

Rich-Harris avatar Nov 27 '22 15:11 Rich-Harris

I left some comments. I also opened a PR against this branch: https://github.com/sveltejs/kit/pull/7841

This should provide some typing improvements for when we're working with the data-sveltekit attributes on DOM elements.

I believe svelte-check package should also be updated allow empty string as an assignable type right?

I have code that looked like this before:

data-sveltekit-preload-data={prefetch ? '' : 'off'}

Which will fail when running npm run check

 Type '"" | "off"' is not assignable to type 'true | "off" | "hover" | "tap" | null | undefined'.

So if i change to

data-sveltekit-preload-data={prefetch ? true : 'off'}

Then the DOM will complain:

Unexpected value for preload-data — should be one of "", "off", "tap", "hover"

smblee avatar Dec 01 '22 20:12 smblee