sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.11k stars 1.84k forks source link

Before and after hooks for routing #552

Closed antony closed 2 years ago

antony commented 4 years ago

Describe the bug onMount is not fired on page change, if the actual page component doesn't change. This is all routes which contain [dynamic] parameters, since the underlying components are the same.

To Reproduce Reproduction here:

https://github.com/beyonk-adventures/sapper-onmount-issue

The colour pages set the value from the path parameters on load, and onMount sets the value in the page. Because onMount isn't refired, the colour never changes when clicking between pages, only on hard refresh, or when visiting the home page and then clicking a new colour.

Essentially it means you can't change the content of the page unless you visit an url which results in a completely different page component being loaded.

Expected behavior Though using the same physical page component, /different/routes and /different/paths which map to /different/[parameter].svelte should fire onMount, since the route is changing.

Information about your Sapper Installation:

Severity Extremely High. I don't actually know how to work around this.

Additional context Fixing this would be a breaking change, but I think it's far more logical an approach than the current behaviour.

Conduitry commented 4 years ago

The component's not re-mounting though. You can listen to changes to the $page store with reactive statements, which is how I think you're supposed to deal with this. I don't think this is a bug and I don't think Sapper should start destroying the old page component and creating a new one when switching between pages that fall under the same route.

antony commented 4 years ago

This seems very odd to me. If my page preloads content, say I'm writing a blog, and that blog has links to related content:

https://example.com/blog/fish/how-to-feed-a-fish https://example.com/blog/fish/how-to-eat-a-fish https://example.com/blog/horse/how-to-feed-a-fish-to-a-horse

If my categories are hardcoded:

/blog/fish/[title].svelte /blog/horse/[title].svelte

If I were to click on a link to a related blog about fish, I'd end up not calling onMount (and therefore not loading the content for the new blog)

If I were to click on a link to a related blog about horses, I'd end up calling onMount (and therefore loading the content for the new blog)

This feels like a seemingly innocuous set of links that do radically different things, and exhibit radically different behaviours, despite appearing identical to both the end user and the developer.

I've added the tag docs to this, but I'm not sure how I'd document it. What would be the rules regarding whether switching route does or doesn't require a subscription to the page store in order to load it's content?

I think this behaviour is mysterious, and (at least for us) has caused quite a serious problem in that pricing for one item remains on the page when you click a new item, since it wasn't obvious that I would need to listen to the page store in order to notify the component that a new page had loaded. It seems like a bit of a gotcha.

Conduitry commented 4 years ago

I feel like part of the problem here is that onMount probably isn't the right place to be loading blog content. If you load it in preload, that will get re-run even on navigation to the same page component, and all the props will get their values updated. onMount also prevents the content from being SSR'd.

antony commented 4 years ago

I think this problem manifested itself because the pricing indicator uses a store, so it's that store which needs to have it's value re-evaluated.

The content is loaded in preload (as per the demo), but once you click a new link, the assigning of a value to it's display variable (emulating population of a store) doesn't happen. I also tried with a reactive variable but the updated content didn't trigger an update.

I'll see if I can reflect the $page idea in the repro above to see if that improves things.

benmccann commented 4 years ago

This works differently than I'd expect as well. Though onMount comes from Svelte and maybe we need Sapper-specific APIs for this? There are a lot of open issues around routing and page transitions (e.g. https://github.com/sveltejs/sapper/issues/902, https://github.com/sveltejs/sapper/issues/814, https://github.com/sveltejs/sapper/issues/1083, https://github.com/sveltejs/sapper/issues/518, https://github.com/sveltejs/sapper/issues/295, https://github.com/sveltejs/sapper/issues/674). It seems like it'd be good to consider all of these together and figure out what API changes if any are needed to address most of the use cases

Rich discusses a lot of the issues raised here in https://github.com/sveltejs/sapper/issues/295

antony commented 4 years ago

I'm trying to work around this using the page store, and the problem is becoming clearer - I don't believe the current behaviour makes any sense. I have a shared component on my page which builds a bunch of stores and puts them in the context.

Setting the context can only be done on initialisation, and if the component doesn't re-initialise (since it's not re-mounted between route-changes), I can't change the items inside the context.

If I'm loading in content and passing in a store to a component, then I don't think there is a reasonable way to switch-out that store, which leaves me in a position that I'm unable to change that component's content.

I've updated the repro app to use the store. The logic seems solid the app straight-forward, but the behaviour just seems wrong. There might be a way to fix it by tying to the page store (which I will attempt next, but if there is, this really doesn't seem like obvious behaviour)

antony commented 4 years ago

After trying to find a reasonable way around this now, I'm 99% erring on this behaviour being a bug / broken feature.

Not having the ability to use onMount and onDestroy means you have to write your own change logic for every component and sub-component which sits underneath your top-level component.

Listening for the $page store to change is fine if you're only one component deep, but the amount of subscribing/reactive declarations you need to get everything in the hierarchy to update just becomes unmaintainable very quickly.

Being able to rely on components mounting and un-mounting is the only sensible way I can think here of restoring state, since if you think about it, a route change is akin to "tearing down" the page component and "mounting" a new page component.

benmccann commented 4 years ago

I mostly agree this isn't the best default as does Rich (in https://github.com/sveltejs/sapper/issues/295 he says "I would argue that destroy/recreate should be the default behaviour")

My main question though is if we want to support both behaviors longer-term, how would we do that? I can think of a few options:

  1. Say that onMount and onDestroy are always called for the last part of a route and if you want something that persists across routes then you should put it in the layout common to that route segment which will be persisted instead of recreated
  2. Say that the user must handle it via pushState and updating the UI on their own
  3. Let onMount and onDestroy operate as they do today, but introduce new APIs like onNavigateTo and onNavigateFrom
    1. That would also allow us to support route guards (though beforeunload might be a better solution for that). E.g. Routify has beforeUrlChange and afterPageLoad. Vue Router has beforeRouteEnter, beforeRouteUpdate, and beforeRouteLeave

Btw, I think the $page store is undocumented because I can't find "$page" in the docs

antony commented 4 years ago

Ah that's a very good spot. I'm absolutely certain that the "method of least surprise" is a route change calling create/destroy.

However I realise this is a breaking change, so we have to decide if we leverage the fact that sapper is pre 1.0 and make this breaking change with a sensible default.

Page store is documented in Svelte and in Sapper (preloading)

I don't feel personally that we need new APIs. I think we can possibly add a flag to the <a> tag which will allow a consumer to not have the old page torn down and recreated, if they really want that behaviour.

it's worth noting that if you use a router which uses <svelte:component> which is a large number of Svelte routers, I think, you do get the onMount and onDestroy calls.

benmccann commented 4 years ago

If you go from /a/b/c to /a/b/d would that only call create/destroy on the last segment and not on the layouts for /a and /b?

We have a couple other PRs for breaking changes as well. I think it'd be nice to have a 0.28.0 that merges any outstanding breaking changes. Sapper doesn't feel like it's ready for a 1.0 yet, and I think locking in all the current behaviors and APIs might be a bit premature.

antony commented 4 years ago

After playing with this for a few days, this is definitely a bug. It might be intentional behaviour, but forcing the user to manage the component lifecycle because they visit two routes which use the same component isn't desirable, it's unpredictable, and it's also extremely difficult since you have to do all sorts of manual checking of state.

My belief is that the most sensible behaviour is that route changes should always cause an onMount and onDestroy, unless explicitly overridden.

I don't think anybody would complain if this behaviour was changed for version 0.28. I believe it is the "API changes" we talk about in the docs and README. It also shouldn't break any existing sites with this as the default, since it's just doing a little extra work, and not removing anything.

Rich-Harris commented 4 years ago

as does Rich (in sveltejs/sapper#295 he says "I would argue that destroy/recreate should be the default behaviour")

"A foolish consistency is the hobgoblin of little minds" — Emerson

I very vaguely recall some conversations about this issue as we switched to <slot>-based layouts way back when, and FWIW the current behaviour definitely was deliberate. The thinking was that aside from being more efficient (since you're not recreating a bunch of stuff only some of which, possibly very little, has changed), it makes it much easier to create appealing page transitions when you're navigating between different pages belonging to the same route:

http://aboard-parent.surge.sh/conditions/green

needle

It's true that we haven't really capitalised on this ability, or done a good job of communicating it. But that's my argument in favour of keeping things as they are. It does make some things a bit harder, but in return it makes other things possible.

Rich-Harris commented 4 years ago

Having said that, I do concede that there's a precedent for exposing options for this sort of thing as attributes on <a> elements (sapper-noscroll, rel="prefetch"). So it could be done there (and maybe rel="prefetch" should become something like sapper-prefetch while we're at it).

But I'd still argue that the current behaviour should be the default and not the option, since a) it's the more efficient way, b) it's working as components ordinarily work (though of course people have asked for an equivalent of React's key for components in the past), and c) I think it's a cool differentiating feature that AFAIK you don't get with other frameworks.

If you go from /a/b/c to /a/b/d would that only call create/destroy on the last segment and not on the layouts for /a and /b?

Definitely think we want to keep this behaviour the same, i.e. layout components persist — I can't imagine a situation where you'd want to recreate the layouts from scratch (and consistency with that is perhaps another argument for keeping the behaviour of the leaf component as it is).

Side-note: While writing this comment I came to regret my giant flashing GIF.

antony commented 4 years ago

I'd be happy with this suggestion, as long as there was some way to make the onMount and onDestroy hooks get called. I can understand the reason behind wanting to be efficient with creating/destroying components.

However I will maintain, and with good reason, that this behaviour is a gotcha and is not obvious. It really doesn't make sense to me that I would have to know an intimate, internal rendering concern of Sapper to determine whether certain parts of my page's content will end up with updated state or not, depending whether the route I came from is the same, or a different physical component. It's doesn't follow Principle Of Least Astonishment. For this reason I'd suggest that this feature/rendering optimisation/gotcha is documented quite clearly.

Side-note: Absolutely regretting your gif.

Conduitry commented 4 years ago

Those are both Svelte things though that are used to call code when the component is created and destroyed, and the component is not getting created or destroyed. We should document this, yes, but we shouldn't give Sapper a way to circumvent intended Svelte behavior (nor, I think, could we, without having Sapper use undocumented Svelte APIs). Is there anything that you can't do with a reactive block subscribing to the $page store?

antony commented 4 years ago

My use case is quite complicated, so it's difficult to really recreate in a simplistic way, but I'll try. Essentially if you're doing any work in onMount or the initialisation block of the page, this work won't be done for the new information when the route changes, so you can end up with stale data on your page, which is completely unexpected and hard to track down.

(In a simplistic form, you could use the $page store to make this data reactive again, but it's completely unexpected to have to do that - it's not something you'd do automatically, you'd have to know that a route change wouldn't re-run your intialisation code unless you were route-changing from a different page component)

Rich-Harris commented 4 years ago

Does 'unexpected' mean 'undocumented', or would this still trip people up even if it was covered well in the docs (and any future tutorial/guide we produce)?

we shouldn't give Sapper a way to circumvent intended Svelte behavior (nor, I think, could we, without having Sapper use undocumented Svelte APIs)

We could do it like this, I think:

const OldLeaf = Leaf;
Leaf = null;
await tick();
Leaf = OldLeaf;

If we were to add this feature as a sapper-* attribute on links, what should the attribute be called?

antony commented 4 years ago

it's going to trip people up regardless, because people are bad at reading docs (counting myself here). However I have less sympathy for people who trip because they haven't RTFM'd.

I am still strongly in favour of adding it as an attribute as suggested. Is sapper- the best way or is sapper: a more consistent prefix? Is sapper:prefetch = rel="prefetch" reasonable?

Either way, I think remount is descriptive enough, so something like:

sapper:remount or sapper-remount

Edit: I've just realised, sapper: won't work as this is part of the Svelte API right? So sapper-remount perhaps.

Rich-Harris commented 4 years ago

I like sapper-remount 👍

benmccann commented 4 years ago

A few things to think about: If we put it on the link then the user has to make sure to add it to every link to the same route. That seems potentially error-prone as I imagine it'd be easy to miss one and would need to be considered every time a new link is added How would programmatic navigation be handled? It doesn't allow the user to have a handler both for the first time the component is mounted and for when the page is navigated to.

The gif example in my view is a pretty rare use case and I'm not sure it's the one I'd choose to optimize the API for. If we always destroyed the old component and mounted a new one on navigation (which is more in line with what I'd expect to happen), I think the user could still build the behavior in the gif within a single component by using pushState

In any case, if we want to keep onMount working as it does today, I think something like Routify's beforeUrlChange would be a friendlier way of handling this. Then you only need to set it up once instead of making sure you consider whether every link on the page needs a new attribute. It also makes it easier to have both types of handlers instead of choosing either/or

Rich-Harris commented 4 years ago

If we put it on the link then the user has to make sure to add it to every link to the same route.

That is a concern, yeah. It's a feature of the component rather than links to the component, but we don't have a great way to express that in-band.

How would programmatic navigation be handled?

app.goto(url, { remount: true });

The gif example in my view is a pretty rare use case and I'm not sure it's the one I'd choose to optimize the API for.

This might just be me tilting at windmills, but to me the fact that it's a rare use case is itself the problem. We web folk have trapped ourselves in the 'pages' mindset, where navigation represents a jump from one discrete thing to another. In native apps and in more pie-in-the-sky sources of inspiration, navigation is often more like exploring space or exploring an object, in a way that emphasises fluidity and continuity and more closely resembles how our brains model the real world.

It's true that my example is slightly contrived — I cobbled it together fairly quickly last night — but I don't think it's so hard to come up with better real world ones. Think about a photo album app where you zoom in from the grid view to an individual photo when you tap it, or where the pixel average background colour behind the photo transitions smoothly, or an ecommerce site like this except with similarly appealing transitions when you navigate between alternatives rather than going from index to detail view, or really any app where you're actually navigating through a space (something like this Sketchfab example, except that the 'annotations' are actually URLs).

I think the user could still build the behavior in the gif within a single component by using pushState

We probably want to avoid cornering developers into using the history API directly, since Sapper is managing the history — it's a likely source of bugs. At the very least I think we'd want to wrap that API (i.e. handle via app.goto).

In any case, if we want to keep onMount working as it does today, I think something like Routify's beforeUrlChange would be a friendlier way of handling this.

Actually yeah, lifecycle functions might be the correct way to handle this. I don't think they need to be implemented as stores as they have been in Routify, they could just be normal custom lifecycle functions AFAICT.

antony commented 4 years ago

Actually yeah, lifecycle functions might be the correct way to handle this.

Would this mean that I would have to consider use of onMount in page components, instead preferring (say) beforeUrlChange, since only one is guaranteed to run when a page loads?

benmccann commented 4 years ago

think about a photo album app, ... an ecommerce site like this, ... something like this Sketchfab example

Good examples! The way I had been thinking this would work if we did remount each component upon navigation is that you would have a layout component (potentially with no UI and just code if that's all you need) that could have it's own onMount. That would end up being run the first time you navigate to the photo album, for example, but not when opening an individual photo. I think that would end up covering both the traditional page-based use case Antony raised as well as these more interactive use cases

Would this mean that I would have to consider use of onMount in page components, instead preferring (say) beforeUrlChange, since only one is guaranteed to run when a page loads?

My thinking was that you would use the one appropriate for the usecase. For the usecase you have where you want it to run everytime a page loads then you'd put the code in beforeUrlChange. If you want something to run just once when first loading the component, but not for individual page navigations, then you'd put it in onMount

Rich-Harris commented 4 years ago

Yeah, I'm envisaging something like this, perhaps:

// @sapper/app
import { onMount, tick } from 'svelte';

// ...`stores`, etc

export function onNavigate(fn) {
  const { page } = stores(); // this works because of when `onNavigate` is called

  onMount(() => { // ditto
    const unsubscribe = page.subscribe(async $page => {
      await tick(); // so that it matches `onMount` semantics following subsequent navigations
      fn(); // or fn($page)?
    });

    return unsubscribe;
  });
}
benmccann commented 4 years ago

Could we also add a function like onLeave or onBeforeNavigate as well? Would it just call fn() without calling tick() first? I haven't used stores much yet...

PatrickG commented 4 years ago

Yeah, I'm envisaging something like this, perhaps:

// @sapper/app
import { onMount, tick } from 'svelte';

// ...`stores`, etc

export function onNavigate(fn) {
  const { page } = stores(); // this works because of when `onNavigate` is called

  onMount(() => { // ditto
    const unsubscribe = page.subscribe(async $page => {
      await tick(); // so that it matches `onMount` semantics following subsequent navigations
      fn(); // or fn($page)?
    });

    return unsubscribe;
  });
}

That would never unsubscribe the onMount listeners, would it? Also, I've created a PR about 6 months ago (https://github.com/sveltejs/sapper/pull/1047), with a onNavigate function to prevent navigation. Maybe this could also resolve this issue.

Or maybe just export a constant in the module script block, like the preload function, to tell sapper that this component should remount.

<script context="module">
  export const REMOUNT = true;
</script>

Then the normal onMount/onDestroy functions would be called on remount with this:

We could do it like this, I think:

const OldLeaf = Leaf;
Leaf = null;
await tick();
Leaf = OldLeaf;
jokin commented 4 years ago

This might just be me tilting at windmills, but to me the fact that it's a rare use case is itself the problem. We web folk have trapped ourselves in the 'pages' mindset, where navigation represents a jump from one discrete thing to another. In native apps and in more pie-in-the-sky sources of inspiration, navigation is often more like exploring space or exploring an object, in a way that emphasises fluidity and continuity and more closely resembles how our brains model the real world.

You might cobbled your example quickly, but in my use case it totally makes sense the way it works right now. I'm developing a TV ui, which has to include and initialize the video tag on reaching the /tv/channel path, and just interact with the tag when you switch channels to /tv/channel2. Subscribing to the page store to get the updated channel number is enough for this case, destroying and recreating everything would be excessive, and in my case that runs on very low performance chips, a dealbreaker.

natevaughan commented 3 years ago

IMO this is 100% a bug and the fact that it accidentally has some benefits is irrelevant. Let me try to convince you all:

The problem is that the developer now has to consider not just where a user is but how she got there (or where she is coming from) to reason about their app. Consider @antony's example where there are 3 views:

https://example.com/blog/fish/how-to-feed-a-fish https://example.com/blog/fish/how-to-eat-a-fish https://example.com/blog/horse/how-to-feed-a-fish-to-a-horse

And the dynamic params at the top level:

/blog/fish/[title].svelte /blog/horse/[title].svelte

This example is very plausible if it is e.g. team and user instead of fish and horse. If the user navigates from horse 1 to fish 1, she will be in one state (new content loaded). If a user navigates from fish 1 to fish 2 directly, she will be in a different/inconsistent state (no new content loaded). But if the user who got to fish 2 via fish 1 refreshes her browser, she'll see different content.

Consider these two functions:

function navigateToFish(title) {
   goto(`/blog/fish/${title}`);
}

function navigateToFish2(title) {
   window.location = `/blog/fish/${title}`;
}

I'd argue that they should behave exactly the same way. The user is navigating to the same view; the developer took the recommendation to use Sapper's goto when handling navigation. But if the developer uses the first function on any fish pages, the two functions will not behave the same way.

IMO the URL represents where the user is regardless of how they got there, and Sapper should default to idempotence: a request produces the same result every time.

To address the examples of a photo album or ecommerce site, I see those as performance optimizations. You presumably know where the user is coming from (another photo or another item of a certain type) and going to, so you want to optimize the transition and possibly add an animation. Sure! Why not just make {remount: true} the default and if you happen to know you're going from one photo to another, you can optimize by passing {remount: false}?

pngwn commented 3 years ago

The problem is onMount is a svelte thing. And the component isn't remounting. So it definitely isn't a bug, 100%.

It might be confusing, it might be undesirable to some extent, but it isn't a bug.

Changing the core behaviour would definitely break stuff that I have built, no question, it would also make those apps impossible to build with sapper. I would personally consider remounting on every route change a step backwards.

I have little opinion on whether or not a way to force a remount would be good for sapper. I see no harm in providing that option if people need it.

natevaughan commented 3 years ago

Changing the core behaviour would definitely break stuff that I have built, no question, it would also make those apps impossible to build with sapper. I would personally consider remounting on every route change a step backwards.

What if the current behavior remained available (as I suggested) by passing {remount: false}?

pngwn commented 3 years ago

I don't think the less performant solution, that circumvents Svelte's default behaviour, should be the default solution.

natevaughan commented 3 years ago

Sapper docs say that goto "Programmatically navigates to the given href." That's not what goto does if a user gets two different results depending on where she is coming from and going to.

pngwn commented 3 years ago

As far as I can tell there isn't much (any?) difference between using an anchor or goto in this scenario. This is just about how sapper handles routes changes in general, which in my opinion is fine. We need to communicate it better, maybe we need solutions to the problems that people are reaching for onMount for, but the behaviour itself is very useful.

I don't think goto needs to emulate a hard refresh, nor do other route transitions. I'm not against configuration to modify this behaviour.

natevaughan commented 3 years ago

Again quoting the Sapper docs here: "In other words, the behaviour is as though the user clicked on a link with this href."

ajbouh commented 3 years ago

I might be misunderstanding what it means to have different results depending on how a user got to a particular page, but isn't this the nature of working in a reactive environment?

There are an almost unlimited number of updates that could get you to a given state, but the compiler/runtime/compute paradigm manage all of these on your behalf.

It seems like there are some holdover expectations about the set of side effects that will precede entering a particular state.

Perhaps there are a set of invariants and/or events we can identify that are a better fit for this reactive world? Is it as simple as having a better set of beforeNavigateHere / beforeNavigateAway events on page components?

Perhaps this would better align the expectations folks have around when onMount is fired?

pngwn commented 3 years ago

Why don't we quote the whole thing:

Programmatically navigates to the given href. If the destination is a Sapper route, Sapper will handle the navigation, otherwise the page will be reloaded with the new href. In other words, the behaviour is as though the user clicked on a link with this href.

So that is clearly talking about a number of things and does not anywhere suggest that the navigation replicates default browser behaviour, except when the url is external to the site. Otherwise it is "as if the user has clicked on a href" which has very specific behaviour when that link is an internal link. Client routers in no way replicate default browser behaviour.

Regardless, we are talking about what is desirable, not how good the documentation is. We have already clarified that the docs could be improved.

antony commented 3 years ago

On my phone so please excuse the brevity.

I believe that the default behaviour is undesirable, despite the performance gain. I believe it's non obvious, and requires deep knowledge of the rendering behaviour to do something as simple as add an href reliably. I absolutely expect the same behaviour navigating to page B, irrespective of where I came from. It seems very odd to have to add special hooks to refresh data on the page in case the user navigated there from somewhere in the same hierarchy.

However, it is the default behaviour, and I'm happy for that to continue to be the case, I just think that this should be made very clear in the docs.

I do however think that some way of overriding this behaviour and unMounting and mounting the whole page is required. I don't have strong opinions on what that looks like though.

On Mon, 27 Jul 2020 at 20:57, pngwn notifications@github.com wrote:

Why don't we quote the whole thing:

Programmatically navigates to the given href. If the destination is a Sapper route, Sapper will handle the navigation, otherwise the page will be reloaded with the new href. In other words, the behaviour is as though the user clicked on a link with this href.

So that is clearly talking about a number of things and does not anywhere suggest that the navigation replicates default browser behaviour, except when the url is external to the site. Otherwise it is "as if the user has clicked on a href" which has very specific behaviour when that link is an internal link. Client routers in no way replicate default browser behaviour.

Regardless, we are talking about what is desirable, not how good the documentation is. We have already clarified that the docs could be improved.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/1278#issuecomment-664606356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABVORIATB27G6RKX67SD4TR5XL2LANCNFSM4N7M2MEA .

--


ꜽ . antony jones . http://www.enzy.org

ajbouh commented 3 years ago

The more I think about it, the more I realize that onMount is actually an implementation detail of sapper and should only rarely be used directly at the page level.

I think we should add two different events to each page. Something like 'visit' and 'leave' or 'load' and 'unload'.

IMO this makes it much more obvious what the page author's intent is, and avoids trying to constrain sapper behavior to be less efficient and possibly violate the principle of least surprise when coming from a svelte mindset.

Would these two events be enough to address the needs that folks are experiencing?

antony commented 3 years ago

They would work for me. And yes, I would feel much better about the whole API that way.

Naming wise I'm not sure.

Enter/exit would be good.

This also provides a good way to handle thmgs like navigation confirmation etc, which has been much requested.

It may also help with page transitions.

On Mon, 27 Jul 2020 at 21:26, Adam Bouhenguel notifications@github.com wrote:

The more I think about it, the more I realize that onMount is actually an implementation detail of sapper and should only rarely be used directly at the page level.

I think we should add two different events to each page, 'visit' and 'leave' or 'load' and 'unload'.

IMO this makes it much more obvious what the page author's intent is, and avoids trying to constrain sapper behavior to be less efficient and possibly violate the principle of least surprise when coming from a svelte mindset.

Would these two events be enough to address the needs thar folks are experiencing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sveltejs/sapper/issues/1278#issuecomment-664619967, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABVORI6B4GTSBYC7FHXCP3R5XPGVANCNFSM4N7M2MEA .

--


ꜽ . antony jones . http://www.enzy.org

pngwn commented 3 years ago

I think something like this is probably the best solution. The problem stems from onMount being a component level primitive, what we need is a page level primitive that we can use to enforce consistent behaviour relating to routes and pages.

Now the page store does exist but I'm guessing some access to the preload values will be needed. They could possibly be passed to the navigation events as arguments. This may also address another issue about onNavigate.

natevaughan commented 3 years ago

I like the idea of enter/exit or load/unload. But IMO Sapper can't ignore onMount.

Here's where I'm coming from: I see Sapper as a wrapper around Svelte. It shouldn't change any of the contracts Svelte sets out; it should only add functionality.

The docs for Svelte say onMount should be used for lazy loaded data that won't be rendered server-side:

It's recommended to put the fetch in onMount rather than at the top level of the Githubissues.

  • Githubissues is a development platform for aggregating issues.