Skip to content

Wait for previous navigation done before navigating to param with new updated value #73

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Wait for previous navigation done before navigating to param with new updated value #73

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1c99087
fix: Wait for previous navigation done before navigating to param wit…
TravelingTice Mar 4, 2024
8c31407
fix: Link unsubbing of store with unsubbing of isNavigating
TravelingTice Mar 21, 2024
4c7d474
test: Add test for the param navigation bug
TravelingTice Mar 21, 2024
3d6e5fe
fix: add ondestroy unsub for param subscribing in playground case
TravelingTice Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/lib/sveltekit-search-params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { browser, building } from '$app/environment';
import { goto } from '$app/navigation';
import { page as page_store } from '$app/stores';
import { navigating, page as page_store } from '$app/stores';
import type { Page } from '@sveltejs/kit';
import {
derived,
Expand Down Expand Up @@ -326,8 +326,28 @@ export function queryParam<T = string>(
const override = writable<T | null>(null);
let firstTime = true;
let currentValue: T | null;

let isNavigating = false;
if (browser) {
navigating.subscribe((nav) => {
isNavigating = nav?.type === 'goto';
});
}
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2024

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The investigation into the usage of navigating.subscribe within src/lib/sveltekit-search-params.ts reveals that there is indeed an explicit unsubscription mechanism in place within the _set function, which addresses the concern about potential memory leaks by ensuring proper cleanup. This demonstrates a level of care in managing subscriptions to the navigating store. However, the initial concern regarding the potential performance impact of multiple subscriptions, especially in applications with frequent navigation updates, remains valid. It's recommended to continue monitoring and optimizing subscription management to the navigating store to mitigate any potential performance issues.

Analysis chain

The implementation of subscribing to the navigating store to set isNavigating based on the navigation type is a clever way to handle the issue of rapid successive navigation commands. However, there are a couple of points to consider:

  • Performance: Subscribing to the navigating store every time the queryParam function is called could potentially lead to performance issues, especially in applications with frequent navigation updates. It might be more efficient to manage the subscription at a higher level or use a shared state to track navigation status.
  • Unsubscription: It's good practice to unsubscribe from Svelte stores when the subscription is no longer needed to prevent memory leaks. In this case, the subscription created on line 332 does not seem to be explicitly unsubscribed from. While Svelte might handle this automatically in some scenarios, explicitly managing subscriptions is generally safer.

Consider optimizing the subscription management to the navigating store to improve performance and ensure proper cleanup.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if there's a shared navigation status management implemented elsewhere in the codebase
rg "navigating.subscribe" --type ts

Length of output: 194

Script:

#!/bin/bash
# Extract more details about the second occurrence of navigating.subscribe and its surrounding code
rg -C 5 "const unsubscribe = navigating.subscribe" --type ts

Length of output: 785


function _set(value: T | null, changeImmediately?: boolean) {
if (!browser) return;

// Wait for previous navigation to be finished before updating again
if (isNavigating) {
const unsubscribe = navigating.subscribe((nav) => {
if (nav?.type !== 'goto') {
_set(value, changeImmediately);
unsubscribe();
}
});
return;
}

firstTime = false;
const hash = window.location.hash;
const toBatch = (query: URLSearchParams) => {
Expand Down