Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Ensure deepest layout is always refreshed on goto #484

Merged
merged 1 commit into from
Feb 1, 2019

Conversation

mrkishi
Copy link
Member

@mrkishi mrkishi commented Oct 18, 2018

See: #349, #483's ps.

This PR just forces the deepest page component to refresh on goto, even if that's goto(currentPage). This ensures that a change to a query param triggers a navigation.

One might argue that goto(currentPage) should be a noop, but I'd argue otherwise for two reasons:

  1. Plain old regular links to the current page aren't noop, and thus users' expectation is that the page will refresh. If you're on hackernews' homepage and click its logo, would you expect it not to refresh revealing the freshest pile of nonsense?

  2. navigate(currentPage) could be implemented as user code to be noop or not on top of a goto that always navigates. The opposite isn't true: if goto(currentPage) is a noop, we can't have a userland navigate that works differently.

This should probably still be polished (especially if #483 is something we'd like to do), but I think it should work for a temporary fix.

@Rich-Harris
Copy link
Member

If the assumption is that goto(currentPage) should cause a refresh, don't we need to destroy and re-render intermediate layout components too?

Also, I'm probably being dumb but I'm not sure what you mean in 2) about being able to implement the existing noop behaviour in userland?

@mrkishi
Copy link
Member Author

mrkishi commented Oct 25, 2018

I don't know what the answer to the first question is. The behavior expected by users is for everything to refresh. But that's also true for navigations.

Imagine you have dynamic data on your top-most layout. Say, a number on the root menu. If you are on the homepage and click on the site's logo, would you expect that number to update?

Old behavior says yes, yes you would. But SPAs and PWAs are all about optimizing these away on a case by case. Sapper needs a way for devs to express how deep a layout it should maintain.


Re. 2- Given a goto(page) implementation that always navigates, one can do:

const my_goto = (page) => page === currentPage ? goto(page) : 👎

Given a goto(page) that does not navigate to currentPage, you can't create a my_goto that does navigate regardless (at least without editing sapper's goto).

@ak5
Copy link

ak5 commented Jan 14, 2019

Ran into this when trying to force a redraw. Can't we do something like goto(page, {force: true})

@Rich-Harris Rich-Harris merged commit fdfe282 into sveltejs:master Feb 1, 2019
@Rich-Harris
Copy link
Member

I think we have the opportunity to fix this more robustly in the Svelte 3 version (see #552), but this makes sense until then — thanks

@Rich-Harris
Copy link
Member

Released 0.25

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants