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

feat: snapshots #8710

Merged
merged 27 commits into from
Feb 6, 2023
Merged

feat: snapshots #8710

merged 27 commits into from
Feb 6, 2023

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Jan 24, 2023

Part of #5478. This adds the ability to capture DOM state immediately before transitioning away from a page, and restoring it if the user traverses back to that page (think bfcache, but with more control).

Any +layout.svelte or +page.svelte component can export a snapshot object:

<script>
  export const snapshot = {
    capture: () => sidebar.scrollTop,
    restore: (y) => sidebar.scrollTo(0, y)
  };
</script>

(The initial proposal was apply, but Copilot suggested restore and I prefer it.)

Things to note:

  • We can't use lifecycle functions for this because we can't reliably match component state across different renders (i.e. it's not possible to know that this instance of <Foo> is the 'same' as that instance of <Foo> the last time the document was rendered), but we can reliably do this for route files
  • The state is persisted to sessionStorage, so it must be JSON-serializable
  • Because we use sessionStorage, state persists across documents, and survives reloads
  • In order to read the snapshot object, we have to set the accessors: true compile option on +layout.svelte and +page.svelte files, courtesy of experimental.dynamicCompileOptions. This will have an impact on bundle size, but a very modest one actually this isn't true, export const creates accessors automatically. neat!

TODO

  • docs
  • update scroll position at the same time as the snapshot (this would fix a subtle bug that no-one has noticed yet — if you scroll the page while SvelteKit is loading the next one, the scroll position that gets saved is the one at the start of the navigation rather than at the moment of transition)
  • figure out types (feat: snapshots #8710 (comment))
  • general tidy up

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

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2023

🦋 Changeset detected

Latest commit: b4a6f25

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

}
const scroll_positions = storage.get(SCROLL_KEY) ?? {};

/** @type {Record<string, any[]>} */
Copy link
Contributor

@tomecko tomecko Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/** @type {Record<string, any[]>} */
/** @type {Record<number, any[]>} */

I see initial_history_index and current_history_index which both seem to be numbers to be used as an index for snapshots.

@Rich-Harris
Copy link
Member Author

Types: since it's the same interface everywhere, we could export it from @sveltejs/kit:

import type { PageData, ActionData } from './$types';
import type { Snapshot } from '@sveltejs/kit';

export let data: PageData;
export let form: ActionData;

export const snapshot: Snapshot = {
  capture: () => 'hello',
  restore: (message) => console.log(message)
};

But if feels a tiny bit awkward compared to having all the types come from the same place:

import type { PageData, ActionData, Snapshot } from './$types';

export let data: PageData;
export let form: ActionData;

export const snapshot: Snapshot = {
  capture: () => 'hello',
  restore: (message) => console.log(message)
};

Also, unless I'm mistaken there's no way to automatically infer T from the return value of capture...

export interface Snapshot<T = any> {
  capture: () => T;
  restore: (snapshot: T) => void
}

...whereas if it's a generated type then we could in theory do something clever and misguided with proxy types so that restore is type safe without the developer having to faff around with generics. (Not now, I mean, but importing from ./$types gives us that flexibility for a later date.) Thoughts?

@dummdidumm
Copy link
Member

It would probably work with a proxy type, but only with a proxy type, which is unfortunate because with satisfies we don't need it in other cases.

If we do add it to ./$types now and add that proxy thing later if there's enough demand, then we need to make sure to do that in a backwards compatible way (if we add it with a generic now, and change it to one without, that's a breaking change).

@Rich-Harris
Copy link
Member Author

if we add it with a generic now, and change it to one without, that's a breaking change

I was imagining something like this, which I think would work non-breakingly?

export type Snapshot<T = ReturnType<typeof import('./proxy+page.svelte').snapshot.capture>> = Kit.Snapshot<T>;

It's a shame that it can't be inferred properly, generics are such a bullshit solution to this problem. (Or can it? Am I missing a trick?)

@dummdidumm
Copy link
Member

Yes, done that way (generic with a default value) it would work in a backwards-compatible way.

We would need some way to say "infer this second property from the first", but I don't think there's a way to do that. It's available for functions (function foo<T>(a: () => T, b: T) {}) but I don't think something like that is possible for objects.

@Rich-Harris
Copy link
Member Author

probably, though export const snapshot wouldn't really fit there either — ultimately there needs to be a reference of what you can export from which file. probably a larger conversation than this PR

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! But I think there's a memory leak here. That index is never deleted, snapshots could grow larger and larger over time. At the same time I'm not sure how that could be prevented, since you could navigate around for quite a while, then go back to the start and expect that snapshot to still be visible. And what happens if you navigate back and forth (not using links but browser back/fwd)? We can't really tell when it's safe to delete it. Do we need some kind of config there? export const snapshots = { ... , deleteAfter: 10 } with a default of X?

packages/kit/src/runtime/client/client.js Show resolved Hide resolved
@Rich-Harris
Copy link
Member Author

There is a memory leak, but it would be almost impossible for it to have an impact — you'd have to have a session with hundreds if not thousands of history entries before it was more than a rounding error, unless you were snapshotting absurdly large objects. It's a memory leak in the same way that scroll restoration causes a memory leak. So my inclination is probably to handwave it away, though we could mitigate it by not storing anything in the case where nothing was snapshotted

@dummdidumm
Copy link
Member

I imagine some internal business app open for a week straight - it could have an impact then. We should at least note something about it in the docs.

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@Rich-Harris
Copy link
Member Author

sessionStorage is finite (though differs from browser to browser), so one solution to the memory leak issue could be to proactively persist snapshots rather than only doing it on visibilitychange. That way, if doing so results in a QuotaExceededError (https://html.spec.whatwg.org/multipage/webstorage.html#storage-2), we could prune older entries. We'd need to understand the performance implications of doing so, but that would give us a sensible upper bound.

@Rich-Harris
Copy link
Member Author

I figured documentation is sufficient for now, though I did realise that we can at least prune stuff in the case where you navigate back then push a new state.

@Rich-Harris Rich-Harris merged commit 63613bf into master Feb 6, 2023
@Rich-Harris Rich-Harris deleted the snapshot branch February 6, 2023 15:33
@github-actions github-actions bot mentioned this pull request Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants