search

paoloricciuti / sveltekit-search-params

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

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

Open TravelingTice opened 5 months ago

TravelingTice commented 5 months ago

This PR tries to solve the bug explained in issue: https://github.com/paoloricciuti/sveltekit-search-params/issues/71

The problem is that navigation is "overwritten" when there is another store update right after the previous one (for example when a component mounts that has a bind:value to a new store that uses queryParam.

The way I managed to solve it is by subscribing to the navigating store provided by Sveltekit. This way you can know if it's currently navigating to a different url or page. Then when this is the case in the _set action, I just subscribe to the same store again there to wait until navigating is back to false, and then perform the same _set action. This solved it for me!

Let me know if this is a viable solution. I'm no expert in Sveltekit's internals so I have no idea if this is a proper/safe fix with or without any side effects. I'm just creating a simple app that doesn't have any server routing. But this did solve it in my case.

Also I'm not sure if the same bug appears in the other provided store builder queryParameters. I'm not using that one but looking at the code it looks very similar, so maybe a similar fix will work there. But not sure. Let me know if this is any good šŸ˜…

Summary by CodeRabbit

changeset-bot[bot] commented 5 months ago

āš ļø No Changeset found

Latest commit: 3d6e5fe0667e6a18875653311a06537ae81c0c32

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

coderabbitai[bot] commented 5 months ago

Walkthrough

The recent update enhances the queryParam function in a SvelteKit application to handle navigation states effectively. It now prevents updates during navigation, ensuring data consistency. A new subscription mechanism has been introduced to manage subscriptions and navigation events efficiently, providing a smoother user experience.

Changes

Files Change Summary
src/lib/sveltekit-search-params.ts Enhanced queryParam with navigation status check to prevent updates during navigation.
playground/src/routes/paramOnNavigateBug/+page.svelte Added functionality to update param2 based on changes in param1 using query parameters.

Related issues

Poem

šŸ‡āœØ
In the code's vast, green field,
A clever rabbit hopped and kneeled.
"Before we leap, let's look," it said,
Ensure no path we tread is tread."
With wisdom and a tiny grin,
It fixed the flow, so smooth within.
šŸš€šŸŒŒ

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with CodeRabbit: - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit-tests for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit tests for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
netlify[bot] commented 5 months ago

Deploy Preview for sveltekit-search-params ready!

Name Link
Latest commit 3d6e5fe0667e6a18875653311a06537ae81c0c32
Latest deploy log https://app.netlify.com/sites/sveltekit-search-params/deploys/65fbd8af5bca2400083c832f
Deploy Preview https://deploy-preview-73--sveltekit-search-params.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

paoloricciuti commented 5 months ago

Hey thanks for the contribution...i think this could be a good idea but i'm a little worried about not unsubscribing from navigating. I think a better approach would be to subscribe to navigating inside each builder function (basically queryParam and queryParameters), store the unsubscribe in a variable so that you can call it togheter with the unsubscribe from the derived store.

If you need guidance on this feel free to hit me up.

Also it would be wonderful if you could write a failing test that pass after the fix, again if you need guidance on this i can help you šŸ˜„

TravelingTice commented 5 months ago

Hey! Thanks and glad you like the suggested approach :) Yes subscribing globally feels a bit tricky indeed, will try to fix that. Also the test sounds good indeed! Haven't explored testing yet here so will do so when I have a bit more time on my hands. Will let you know if I run into any trouble

Also do you know why Netlify action is failing? Seems to be having trouble installing dependencies for some reason..

paoloricciuti commented 5 months ago

Hey! Thanks and glad you like the suggested approach :) Yes subscribing globally feels a bit tricky indeed, will try to fix that. Also the test sounds good indeed! Haven't explored testing yet here so will do so when I have a bit more time on my hands. Will let you know if I run into any trouble

Also do you know why Netlify action is failing? Seems to be having trouble installing dependencies for some reason..

I think they are failing because you are not in my organization but should not be a problem at all. Let me know if you need any help with testing or in general to fix this.

paoloricciuti commented 5 months ago

Actually is failing because I've updated sveltekit and netlify project is still running on node 16...will fix that

TravelingTice commented 5 months ago

Hmm went a bit further with this one and it looks like the current solution won't solve it in the case described in https://github.com/paoloricciuti/sveltekit-search-params/issues/71. The problem is that in my app, the new query param would mount after the other one was updated. But in this case, 1 param directly depends on and is subscribed to the updates of the other param. Making updating really fast. Too fast for navigating to even start working. See some debug logs locally:

image

In the playground test case I added, navigating gets triggered for both param updates, before navigating state is updated

image In my app, subscribing happens after navigating has started, meaning current fix will be working

I'm pretty deep in it but unfortunately not sure how to solve this one šŸ˜“

paoloricciuti commented 5 months ago

Hmm went a bit further with this one and it looks like the current solution won't solve it in the case described in #71. The problem is that in my app, the new query param would mount after the other one was updated. But in this case, 1 param directly depends on and is subscribed to the updates of the other param. Making updating really fast. Too fast for navigating to even start working. See some debug logs locally:

image

In the playground test case I added, navigating gets triggered for both param updates, before navigating state is updated

image In my app, subscribing happens after navigating has started, meaning current fix will be working

I'm pretty deep in it but unfortunately not sure how to solve this one šŸ˜“

Mmm interesting...i will try to investigate this a bit myself. This is definitely a bit of an edge case but it's definitely something worth investing into fixing.

paoloricciuti commented 5 months ago

Ok i've investigating this a bit and yeah there's no way to wait for the navigation without disrupting the rest of the flow and there's no way to know that you want to do that just by looking at the assignment. Maybe the best option would be another api. I will try as soon as i have time.

TravelingTice commented 4 months ago

Yeah it's definitely a very tricky one! I wish I had some more time to look into it as well.. hope you can find a nice solution..