Before and after hooks for routing #552
Comments
|
The component's not re-mounting though. You can listen to changes to the |
|
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 If my categories are hardcoded: /blog/fish/[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. |
|
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 |
|
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. |
|
This works differently than I'd expect as well. Though Rich discusses a lot of the issues raised here in sveltejs/sapper#295 |
|
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) |
|
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 Listening for the 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. |
|
I mostly agree this isn't the best default as does Rich (in sveltejs/sapper#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:
Btw, I think the |
|
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 it's worth noting that if you use a router which uses |
|
If you go from 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. |
|
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 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. |
"A foolish consistency is the hobgoblin of little minds" — Emerson I very vaguely recall some conversations about this issue as we switched to http://aboard-parent.surge.sh/conditions/green
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. |
|
Having said that, I do concede that there's a precedent for exposing options for this sort of thing as attributes on 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
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. |
|
I'd be happy with this suggestion, as long as there was some way to make the 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. |
|
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 |
|
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 (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) |
|
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 could do it like this, I think: If we were to add this feature as a |
|
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 Either way, I think
Edit: I've just realised, |
|
I like |
|
A few things to think about: 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 In any case, if we want to keep |
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.
app.goto(url, { remount: true });
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).
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
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. |
Would this mean that I would have to consider use of |
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
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 |
|
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;
});
} |
…nt is not recreated Addresses sveltejs/sapper#1278
|
Could we also add a function like |
That would never unsubscribe the 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:
|
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. |
|
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:
And the dynamic params at the top level:
This example is very plausible if it is e.g. Consider these two functions: 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 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 |
|
The problem is 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. |
|
@Rukkaitto That doesn't seem to work for me. I want to refresh my auth token on any route change. Here's my code so far: <script>
import { stores } from "@sapper/app";
import { onMount } from "svelte";
import Nav from "components/Nav.svelte";
import Footer from "components/footer.svelte";
import Sidebar from "components/sidebar.svelte";
import { jsonHeaders } from "./server/_helpers";
// export let segment;
let sidebarShow = false;
const { session, page } = stores();
const refreshToken = async () => {
// only refresh when signed in (token is not empty)
if (!!$session.token) {
console.log("Old token: " + $session.token);
const res = await fetch("/server/auth/refresh", {
method: "POST",
headers: jsonHeaders({}),
});
const result = await res.json();
const { token } = result;
$session.token = token;
console.log("Refreshed token: " + $session.token);
}
};
$: if (page) {
refreshToken();
}
</script>I place it at Anyone got a solution? This is working only when I refresh the page. I also agree with the OP of this issue. Maybe create an equivalent of |
|
This just bit me when implementing a /user/[id] route. The admin user can edit details of any user, and a normal user can edit their own details. When I click from editing a user to the "settings" link so that admin to edit own details, nothing happens, due to this issue. It's only by good luck that I happened to click in that order and noticed this at all. Information security and privacy is more important than efficiency, and I don't want there to be any possibility that residual data from editing one user can accidentally transfer over to when I am editing another user. The natural expectation is that onMount should be called each time I go to a different route or URL, including different parameters. Each route or virtual page should be a fresh instance of the page without any data left over from the previous route. I think that this safe and unsurprising behaviour should be the default, or at least we need some option to make it the default. As it is, I'm not too sure how to fix my page. I didn't design it thinking that its parameters might stealthily change underneath it. Is there some function I can call to remount the page when I detect this circumstance? In addition to just making it work, I am concerned about the possibility of information leaking between the different user pages, so I'd rather "reload" the page than attempt to make it adjust everything when the parameters change, but I don't want to call location.reload() if there's some other option. I haven't seen any general-purpose and concise working solution above. My current "solution" is an ugly hack: |
benmccann
changed the title
onMount is not fired when changing route, if the route changes only dynamic parameters
benmccann
added
the
p1-important
|
I just bumped into the same problem. I read that changing the behavior is a breaking change. This issue is not on the 1.0 milestone though. Shouldn't it be on the 1.0 milestone? I bumped into the problem by loading data based on a page slug in the onMount function. I used |
I totally understand where you're coming from here. I agree 100% that the web has a serious disconnect from other forms of applications when it comes to user experience. The web is, by and large, devoid of continuity and that is unfortunate. There are some legit reasons for this but hopefully we are at, or nearing, the point where this can be rectified. Your example looks great, especially for such a quick spike, by the way. I think it illustrates what we've been missing in this space. However, it is also restricted to the same page/route. If the expectation is for the web to evolve and become more in line with native experiences, then we need to look at the application as a whole, yea? Presumably, most applications are going to operate with a lot more than a single route. This means that for that sort of experience, the optimizations of component mount cycles either can't be a deciding factor or they need to be moved up a layer. I think layouts are often over-used but this seems to sit squarely in their domain. A page shouldn't be expected to know how to transition to every other page. There is an expectation, rightfully or otherwise, that a "Page" is an isolated, end-to-end process. By having circumstances where hooks are not fired upon is alien to the terrain. It seems counter intuitive to need to employ defensive programming to ensure those lifecycle events occur or have occurred. Now, having said all of that and setting aside the performance improvements, there are usecases for the current behavior. The best example that comes to mind is infinite scrolling, where the current cursor is represented in the URL. However, in that case, it probably makes more sense to avoid
|
|
I've encountered the same problem....what's the status of this "bug"? Just to give an idea of another scenario, this is my use case: |
Please don't ask questions like this as it doesn't add anything except noise to our inboxes. I'd rather spend time contributing to the project than managing my inbox. The issue is open, which indicates the status. Anyone who would like to take a stab at implementing this is free to send a PR |
|
Given how many people find this behavior counterintuitive and the decision to keep the behavior it'd probably be a good idea to add an entry to the FAQ explaining why it happens and some workaround recommendation (or a link to this issue, which has a few workarounds). |
|
Hey, I see that this issue was closed but I couldn't figure out what was the resolution? Secondly, I have a little problem with the suggested load() function workaround when using TypeScript. I have a variable It uses props data and component data. Doing it this way, I don't need to explicitly specify the type of
|
|
The maintainers have been having some discussion about whether a new component should be created when you navigate to a new URL corresponding to the same route and are looking for input from the community: #5007 |
Addresses a longstanding workaround where I was dropping in links between lessons in the svx files rather than at the [slug] level. This was due to a bug/feature (see here for context: sveltejs/kit#552) where the links were getting all messed up. By wrapping the component in a `#key` logic block, ensuring it's recreated any time the slug changes, they don't any more. Best practice? Who knows, but it seems to work
and others
Describe the bug
onMountis 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/routesand/different/pathswhich map to/different/[parameter].svelteshould 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.
The text was updated successfully, but these errors were encountered: