Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace data-sveltekit-prefetch/prefetch(...)/prefetchRoutes(...) #7776

Merged
merged 34 commits into from
Nov 28, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 23, 2022

Migration guide

Rename all occurences of data-sveltekit-prefetch in your code base to data-sveltekit-preload-data:

-<div data-sveltekit-prefetch>..</div>
+<div data-sveltekit-preload-data>..</div>

One occurence of this is most likely inside of app.html, if you started from the default template recently.

Instead of hover you can now specify tap, which will only trigger a load when the user taps on a link.

You can also now add data-sveltekit-preload-code, which will import all the JavaScript (and CSS, if necessary) for links. It can have the same tap or hover values, plus viewport (preload links as they become visible) or page (preload everything on the page after each navigation).

The prefetch('/some-route') function imported from $app/navigation is now preloadData('/some-route'):

-import { prefetch } from '$app/navigation';
+import { preloadData } from '$app/navigation';

-prefetch('/some-route');
+preloadData('/some-route');

The prefetchRoutes(['/some-route', 'some-other-route']) function is now preloadCode('/some-route', '/some-other-route'):

-import { prefetchRoutes } from '$app/navigation';
+import { preloadCode } from '$app/navigation';

-prefetchRoutes(['/some-route', 'some-other-route']);
+preloadCode(['/some-route', 'some-other-route']);

PR description

Closes #7289.

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

@changeset-bot
Copy link

changeset-bot bot commented Nov 23, 2022

🦋 Changeset detected

Latest commit: 2553a9d

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

@Rich-Harris
Copy link
Member Author

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

@elliott-with-the-longest-name-on-github
Copy link
Contributor

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.

@dummdidumm
Copy link
Member

  • 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.

@Rich-Harris
Copy link
Member Author

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.

@benmccann
Copy link
Member

benmccann commented Nov 23, 2022

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.

@Rich-Harris
Copy link
Member Author

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'.

@PatrickG
Copy link
Member

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.

@dummdidumm
Copy link
Member

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?

@Rich-Harris
Copy link
Member Author

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?

@smblee
Copy link

smblee commented Dec 1, 2022

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"

@sifferhans sifferhans mentioned this pull request Dec 16, 2022
9 tasks
524c added a commit to 524c/flowbite-svelte that referenced this pull request Dec 29, 2022
outdated code (breaking change)
reference: sveltejs/kit#7776
shinokada pushed a commit to themesberg/flowbite-svelte that referenced this pull request Dec 29, 2022
outdated code (breaking change)
reference: sveltejs/kit#7776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make data-sveltekit-prefetch work with mousedown
7 participants