kit icon indicating copy to clipboard operation
kit copied to clipboard

fix: ensure `preloadCode` without arguments preloads all routes

Open dummdidumm opened this issue 2 years ago • 5 comments

fixes #8469

This was possible previously, and the function signature indicated that this is possible, therefore marked this as a fix

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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

dummdidumm avatar Jan 12 '23 11:01 dummdidumm

🦋 Changeset detected

Latest commit: 3e03b0bea3ce04da73def268dc73835dea05833e

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 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 Jan 12 '23 11:01 changeset-bot[bot]

This was a deliberate change — preloadCode(...paths) having totally different behaviour if paths happens to be an empty array as opposed to an array with a single member is a really weird API. It's also a footgun — on larger sites, this could be a huge amount of code that gets downloaded and executed (and can never be evicted from the module cache). This is what service workers are for.

Open to debate, of course, but I'm 👎 on this

Rich-Harris avatar Jan 13 '23 03:01 Rich-Harris

The issue this would close states that this is a small site where it's benefitial to just load everything in one go. Angular has a built-in behavior that does the same by toggling a boolean. I wouldn't want people to having to got out of their way to achieve this, "use a service worker" is not a good workaround IMO.

Given that I want some form of this to land - how could the API look like? Personally loading this without arguments makes intuitively sense to me, and people who accidentally do this - well... If you really don't want the "no argument" case, what about using * as a reserved string to signal this? preloadCode('*') can't match any routes (has to begin with /) so it's safe to use, and it's similar to kit.prerender.entries.

dummdidumm avatar Jan 13 '23 08:01 dummdidumm

* could work. Though maybe /* already does? Not sure if wildcards are limited to a single segment. An alternative could be a separate API like preloadAllCode, similar to invalidate vs invalidateAll, though I think I prefer the other ideas.

Regardless, it is still a footgun. I would be happier if we simply demonstrated how to achieve it with a service worker - it's not difficult. 'Angular does it' is not a test we should be using :)

Rich-Harris avatar Jan 13 '23 12:01 Rich-Harris

Everything's a footgun at some point, I think we should allow this for people who want to opt into it. /* wouldn't work because it matches the string against the route id regex, so that would only catch /[stuff] at the top level. I'm going to implement the * idea.

dummdidumm avatar Jan 13 '23 13:01 dummdidumm

If we do ultimately decide to go ahead with this (which I don't have especially strong opinions about), we need to make sure we update the PR title and the changeset.

Conduitry avatar Jan 17 '23 21:01 Conduitry

good catch — updated the PR title/changeset

Rich-Harris avatar Jan 17 '23 21:01 Rich-Harris