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

onDestroy not called after goto #5434

Open
daveychu opened this issue Jul 8, 2022 · 5 comments
Open

onDestroy not called after goto #5434

daveychu opened this issue Jul 8, 2022 · 5 comments
Labels
bug Something isn't working
Milestone

Comments

@daveychu
Copy link

daveychu commented Jul 8, 2022

Describe the bug

onDestroy not called if the template has a transition based on a store variable that gets assigned to after calling goto.

In our real scenario it was caused by a programming error, but the cause was hard to track down because of multiple layers of derivations. Not sure if there's ever a valid reason for this situation, but confusing if you make the same mistake.

Reproduction

  1. Clone https://github.com/daveychu/sveltekit-ondestroy-not-called-after-goto
  2. npm run dev -- --open
  3. Click "Go to page 2"
  4. "pages pile up one after the other on the bottom as I continue navigating back and forth, with no page ever disappearing" if you continuously press the most bottom button -> New page of different layout loaded and displayed before out animation is finished #4772 (comment)

Expected: Only shows page 2
Actual: Shows both pages (or more if you keep going) and a transition on page 1

Logs

No response

System Info

System:
  OS: macOS 12.4
  CPU: (10) arm64 Apple M1 Pro
  Memory: 296.48 MB / 16.00 GB
  Shell: 5.8.1 - /bin/zsh
Binaries:
  Node: 16.14.0 - ~/.nvm/versions/node/v16.14.0/bin/node
  npm: 8.3.1 - ~/.nvm/versions/node/v16.14.0/bin/npm
Browsers:
  Chrome: 103.0.5060.114
  Safari: 15.5
npmPackages:
  svelte: ^3.44.0 => 3.49.0

Severity

annoyance

Additional Information

No response

@fernandolguevara
Copy link
Contributor

fernandolguevara commented Jul 10, 2022

@daveychu a workaround, use the $navigating store to stop the animation and detach the node

<script>
  import { navigating } from "$app/stores";
</script>

{#if !$navigating && $store.show}
  <p transition:fly={{ y: 100, duration: 2000 }}>Peekaboo!</p>
{/if}

@Robak08
Copy link

Robak08 commented Jul 26, 2022

@daveychu I think You can try local transitions as well. These used to work back in Sapper times.

{#if $store.show}
  <p transition:fly|local={{ y: 100, duration: 2000 }}>Peekaboo!</p>
{/if}

@Rich-Harris Rich-Harris added the bug Something isn't working label Sep 6, 2022
@Rich-Harris Rich-Harris added this to the whenever milestone Sep 6, 2022
@dummdidumm
Copy link
Member

I believe this is a Svelte core issue, and I vaguely remember having commented on a similar issue about a minimum reproducible independent of SvelteKit, but I can't find it right now

peterpeterparker added a commit to dfinity/nns-dapp that referenced this issue Jan 21, 2023
)

# Motivation

We noticed a weird issue that leads - under edge case conditions - to the main "Login" elements not being removed from the DOM after navigation - i.e. the route being not fully destroyed.

This looks pretty similar to SvelteKit known issue sveltejs/kit#5434

# Preconditions

1. not the fastest login flow - i.e. after II has granted the authorization, fetching JS and navigating from "Login" to "Accounts" take a bit of time. I simulated this by adding 2000ms of delay in the `auth.store` that handles `onSuccess` callback

2. user on purpose does not stay in NNS-dapp tab while the sign-in is in progress

3. no JS should be cached

# Reproduction flow

- open a browser in incognito (it seems to happen more often with Safari but locally could reproduce with Chrome too)
- go to google.com
- open new tab with nns-dapp
- click refresh because sw holds
- click sign-in
- click refresh in II tab because sw holds
- sign- in in II
- when it comes back to nns-dapp tab, quickly switch to google tab before sign-in process over
- wait a bit
- go to nns-dapp tab

# Changes / Workaround

- force the layout of the route that does not gets removed to be displayed only when navigation is over
peterpeterparker added a commit to dfinity/nns-dapp that referenced this issue Feb 7, 2023
# Motivation

For the same SvelteKit issue as for the "Login" page we noticed that on destroy of the CkBTCWallet page was not called. 
This fixes the issue for that particular page. We might want to have  a look to apply it to all routes later.

# Issues

SvelteKit issue sveltejs/kit#5434

# Changes

- destroy layout and all children on navigation to "force" the destroy of the components
@peterpeterparker
Copy link

We hit this in multiple routes now. Did not tried to reproduce it in a sample repo (I should/will try when got a bit of time) but I noticed two following things:

  1. it happens only with prod build. in local development it's all fine
  2. we are lazy loading manually some components there not sure if it can be linked

Above workaround, {#if $navigating}...., work for us too.

github-merge-queue bot pushed a commit to dfinity/nns-dapp that referenced this issue Jul 30, 2023
# Motivation

Refactoring the navigation spinner in #2962 within the component that
prevents SvelteKit issue
[#5434](sveltejs/kit#5434) actually also lead
to the same issue of duplicating the `split-pane` in the DOM. Therefore
we have to undo the refactor and keep the spinner within the `+layout`
of the sign-in page.

# Reproduce

- Open the wallet (e.g. of the main account)
- Make a transaction
- Click the back button
- Where the menu button should now reappear, there is only a
non-functional back button

# Changes

- revert moving spinner from login layout to navigation guard
   - remove spinner from nav component
   - redo spinner in +layout of login (same code as on mainnet)
@austerj
Copy link

austerj commented Nov 26, 2023

I ran into this issue when breaking out of layout groups with global transitions. Open popovers and sometimes entire pages would stick around and stack after navigating to routes in another group.

A silly workaround I found for it was to add a group: 'groupName' prop in the corresponding (groupName)/+layout.ts load function and wrap the entire (groupName)/+layout.svelte content in {#if $page.data.group === 'groupName'}...{/if}.

This seems to force the "clean-up" of the initial layout when navigating to a route in another layout group.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants