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

treat invalidation as navigation #6522

Closed
wants to merge 5 commits into from
Closed

treat invalidation as navigation #6522

wants to merge 5 commits into from

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Sep 2, 2022

Update: since #6537 was merged, the only change this PR makes is to treat invalidation is equivalent to navigation for the purposes of beforeNavigate, afterNavigate and navigating. Leaving it here until we make a final decision, but there's some valid scepticism about whether this is the right move.

Original PR message follows.


closes #6517
closes #4383
closes #6498
closes #6375
possibly #4447 too?

This adds a type property to the Navigation object found in various places:

import { beforeNavigate, afterNavigate } from '$app/navigation';
import { navigating } from '$app/stores';

beforeNavigate(({ from, to, type }) => {
  console.log('beforeNavigate', type);
});

afterNavigate(({ from, to, type }) => {
  console.log('afterNavigate', type);
});

$: console.log($navigation?.type);

The type is one of the following:

  • load — we just got here for the first time. from is null in this case
  • link — the user clicked a link
  • goto — the app called goto programmatically
  • popstate — the user hit the back/forward button, or we're traversing the history some other way
  • invalidation — the app called invalidate(...) or invalidateAll(). This is a breaking change — invalidations weren't previously treated as navigations
  • unload — the page is being unloaded because the user clicked an external link, or submitted a form, or reloaded the page, etc. to will be null in this case

In the popstate case, there's an additional delta property, where -1 means navigating back and +1 means navigating forwards (but this can be any non-zero integer).

Todo

  • docs

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 Sep 2, 2022

🦋 Changeset detected

Latest commit: 830c0cb

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

@netlify
Copy link

netlify bot commented Sep 2, 2022

Deploy Preview for kit-demo ready!

Name Link
🔨 Latest commit 830c0cb
🔍 Latest deploy log https://app.netlify.com/sites/kit-demo/deploys/631377bd07c3950008b68218
😎 Deploy Preview https://deploy-preview-6522--kit-demo.netlify.app/build
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@aradalvand
Copy link
Contributor

Correct me if I'm wrong but I believe this also closes #6375

@Rich-Harris
Copy link
Member Author

Yep, you're right — nice

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.

All good implementation-wise, therefore approving, but not 100% sure about adding invalidating to it yet.

@madeleineostoja
Copy link

madeleineostoja commented Sep 4, 2022

I can't think of a good API for this so take with a grain of salt, but IMO the only acceptable way for this to become part of navigation is if it was opt-in somehow. Requiring users to guard against type !== 'invalidation' in all their routing calls is asking for a footgun and even if you knew about it would get tiring real fast.


invalidating = null;
force_invalidation = false;
stores.navigating.set(null);
Copy link
Member

Choose a reason for hiding this comment

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

Can this result in a wrong navigation state when the user clicks on a link while the invalidating is in progress?
Would probably only be noticeable on a slow connection.

@aradalvand
Copy link
Contributor

aradalvand commented Sep 4, 2022

I agree with @madeleineostoja, so what about something similar to @lukaszpolowczyk's suggestion where beforeNavigate and afterNavigate optionally receive a second argument that lets the developer decide whether they want invalidation to be treated as navigation or not?

beforeNavigate(({ type }) => {
    // show loader — now `type` could also be `invalidation`
}, { includeInvalidation: true });

This wouldn't help in the case of the $navigating store, so that's a bummer.

The only other alternative I guess is to have a different pair of lifecycle function specific to invalidation:

beforeInvalidate(() => {
    // foo bar buzz
});
afterInvalidate(() => {
    // foo bar buzz
});

@madeleineostoja
Copy link

I think $navigating is less of an issue, since you're less likely to be using it to perform an action that you don't want an invalidation trigger.

I like that proposal. It's a little clunky, but much safer at the cost of a little verbosity. I'd name the option something shorter but that's neither here nor there.

@Rich-Harris
Copy link
Member Author

I'm going to close this since the consensus (and I think I agree) seems to be against this solution. $invalidating, beforeInvalidate, includeInvalidation or any other API approach we settle on can be added in a non-breaking way later

@Rich-Harris Rich-Harris closed this Sep 5, 2022
@Conduitry Conduitry deleted the gh-6517 branch September 5, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants