paoloricciuti / sveltekit-search-params

The easiest way to read and WRITE from query parameters in sveltekit.
https://sveltekit-search-params.netlify.app
MIT License
497 stars 15 forks source link

Unsubscribing #3

Closed nick-jonas closed 1 year ago

nick-jonas commented 1 year ago

I noticed when I go to different page routes my search params are persisting, and I think it's because I need to unsubscribe() during an onDestroy() lifecycle hook. Would you consider returning the subscription that you have here?

paoloricciuti commented 1 year ago

Sure, i'll take a look into this...also i don't think you need to manually unsubscribe if you use the $

paoloricciuti commented 1 year ago

P.s. can you provide a quick reproduction step? Are you navigating with a goto? With a simple link?

nick-jonas commented 1 year ago

Hmm maybe I'm doing something wrong. I have this in a child component of a +page.svelte:

const languageQueryParam = queryParam(isSourceLang ? 'sl' : 'tl', {
        encode: (value) => value.language || 'auto',
        decode: async (value) => {
            console.log('this still triggers when the .svelte component it lives in is destroyed');
            // wait for all languages to be loaded before decoding
            await langsLoaded.load();
            // handle param for auto detect
            if (isSourceLang && (value === 'auto' || !value)) {
                $selectedSourceLang = { detect: true };
            }
            // set target language to default if it's not in the query param
            else if (!isSourceLang && !value) {
                $languageQueryParam = getLanguageObjFromId($defaultTargetLanguage.text);
            }
            // there is a value set in query param
            else if (value) {
                const paramLangObj = getLanguageObjFromId(value, isSourceLang);
                if (paramLangObj) {
                    if (isSourceLang) {
                        $selectedSourceLang = paramLangObj;
                    } else {
                        $selectedTargetLang = paramLangObj;
                    }
                }
            }
        }
    });

When a navigation button is pressed and navigates away via on:click={() => goto('/anotherpage', {invalidateAll: true})}, the queryParam stays and the decode method still fires.

Do you have a recommended approach here?

paoloricciuti commented 1 year ago

Got it...yeah it's probably because you never unsubscribe from page. The proper fix tho should be overriding the subscribe function of the actual store to unsubscribe also from the page store. I'll try to handle this as soon as i can and push a fix.

paoloricciuti commented 1 year ago

It should be fixed in the 0.1.7 version...let me know if it's fixed on your side after an update.

Also regarding your code i think it has a couple of problems:

  1. you are checking for isSourceLang while initializing the store but if that's a reactive variable the store will not be reinstantiated on change. It should either be put in a reactive statement or extrapolated in two different queryParam calls.
  2. i would honestly avoid large async operations on decode since it's called pretty often. I don't know the specific case of your application but i would go ahead and stick that code in a reactive statement and read the value from the query params as is (as a string) while also using it in the reactive statement (so that it reruns whenever the query params change).
nick-jonas commented 1 year ago

Thanks @paoloricciuti - the decode method is still being called after the component is destroyed however, is there something I need to explicitly do in the onDestroy() lifecycle method for the component hosting the queryParam object?

  1. isSourceLang is simply a boolean parameter passed in from the parent (I create two of these components, the parent sets one to true and another to false. They never change. So that should be fine, right?
  2. I've done this now:
    const languageQueryParam = queryParam(isSourceLang ? 'sl' : 'tl', {
        encode: (value) => value.language || 'auto',
        decode: (value) => {
            console.log('this still triggers when the .svelte component it lives in is destroyed');
            return value;
        }
    });
paoloricciuti commented 1 year ago

Mmm that seems strange...do you have a repo I can look at?

paoloricciuti commented 1 year ago

In the meantime if you want to take a look here's my small reproduction that doesn't seem to be affected from the bug you describe. Did you update to 0.1.7? Is there something you are doing differently?

https://stackblitz.com/edit/sveltejs-kit-template-default-jpgn4o

nick-jonas commented 1 year ago

I can't figure it out for the life of me, it's so strange.

Thank you for putting together that repo, I can't unfortunately show you my exact repo but I forked what you made to try and replicate the important bits:

https://stackblitz.com/edit/sveltejs-kit-template-default-wv8uvw?file=src/routes/+page.svelte

I'm using 0.1.7.

Here is the block again:

    const languageQueryParam = queryParam(isSourceLang ? 'sl' : 'tl', {
        encode: (value) => value.language || 'auto',
        decode: (value) => {
            console.log('Decode queryParam');
            return value;
        }
    });

Correct me if I'm wrong, but the only way decode() can be called is if the page is reloaded or $languageQueryParam is set. I put a log() statement everywhere that $languageQueryParam is set in the code, so it shouldn't be that...

I suppose it's not breaking anything ATM but it's really strange...let me know if you have any further debugging ideas!

paoloricciuti commented 1 year ago

It actually run whenever the query params change. So even if something else is changing the query param it will rerun decode.

Btw I've tried your repro and it doesn't seems to log more than it should...can you provide a reproduction step list?

nick-jonas commented 1 year ago

Not exactly sure what I did, but somehow it's not happening in my app now! Thanks for being timely and following this :)

paoloricciuti commented 1 year ago

Not exactly sure what I did, but somehow it's not happening in my app now! Thanks for being timely and following this :)

It might have been some cache problem maybe. Glad it worked in the end and thank you for reporting the bug.