breaking: remove double navigation, debounce navigate after timeout #60
Conversation
feat: add showDefaults option to chose wether to show the defaults or not in the URL
🦋 Changeset detectedLatest commit: 9af4ffb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe project introduced a Changes
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 X ? TipsChat with CodeRabbit Bot (
|
✅ Deploy Preview for sveltekit-search-params ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .changeset/kind-horses-melt.md (1 hunks)
- README.md (1 hunks)
- playground/src/routes/debounce/+page.svelte (1 hunks)
- playground/src/routes/default/+page.svelte (1 hunks)
- src/lib/sveltekit-search-params.ts (8 hunks)
- tests/index.test.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- .changeset/kind-horses-melt.md
Additional comments: 17
README.md (1)
- 398-422: The documentation for the
showDefaultsoption is clear and provides useful examples for its implementation. This aligns with the PR's objectives to manage the display of default values in the URL.playground/src/routes/debounce/+page.svelte (1)
- 9-9: The
str2parameter is initially set as a boolean, but later it is assigned a string value. Ensure that this is the intended behavior and that the parameter type is handled correctly throughout the application.playground/src/routes/default/+page.svelte (3)
5-7: The
str_no_showparameter is correctly configured to not show its default value in the URL.11-16: The
store_no_showobject is correctly set up to handle thestr2-no-showparameter with theshowDefaultsoption set to false.2-27: Ensure that the rest of the application correctly handles the absence of
str-no-showandstr2-no-showfrom the URL when their values are set to defaults.src/lib/sveltekit-search-params.ts (9)
37-40: The addition of
showDefaultstoStoreOptionsaligns with the PR objectives to control the visibility of default values in the URL.53-59: The introduction of
Overrides<T>type and its parameter inmixSearchAndOptionsfunction is a good approach to allow specific overrides to the default behavior.73-75: The logic to apply overrides in
mixSearchAndOptionsis correctly checking for undefined values before applying them.168-175: The
queryParametersfunction has been updated to includeshowDefaultsinStoreOptionsand to support debouncing. Ensure that the debouncing mechanism works as intended and that theshowDefaultsoption behaves correctly.213-259: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [180-231]
Verify that the
changeImmediatelyparameter is always defined when calling_setwithinqueryParameters. If it's not defined, it could lead to unexpected behavior.
237-251: The derived store in
queryParameterscorrectly uses themixSearchAndOptionsfunction and applies theshowDefaultslogic. Ensure that theshowDefaultsoption behaves as expected when default parameters are present.270-278: The
queryParamfunction has been updated to includeshowDefaultsinStoreOptionsand to support debouncing. Ensure that the debouncing mechanism works as intended and that theshowDefaultsoption behaves correctly.304-353: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [282-322]
Verify that the
changeImmediatelyparameter is always defined when calling_setwithinqueryParam. If it's not defined, it could lead to unexpected behavior.
- 329-350: The derived store in
queryParamcorrectly uses themixSearchAndOptionsfunction and applies theshowDefaultslogic. Ensure that theshowDefaultsoption behaves as expected when default parameters are present.tests/index.test.ts (3)
- 320-385: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [311-335]
The test case correctly checks for the absence of parameters when
showDefaultsis false, ensuring that the URL is clean of default parameters that are not supposed to be shown. This aligns with the PR objective to introduce theshowDefaultsoption.
338-382: The test case for the debounce functionality seems to be well-implemented. It waits for the debounce timeout before checking the URL parameters, which ensures that the navigation function is debounced as intended. This aligns with the PR objective to implement a debounce mechanism for navigation.
391-394: The test case for default values during server-side rendering (SSR) checks that the values are present even when JavaScript is disabled. This ensures that the default values are handled correctly in SSR scenarios.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/index.test.ts (7 hunks)
Additional comments: 6
tests/index.test.ts (6)
87-87: The addition of an assertion to check the text content of an element after a change is a good practice to ensure that the UI reflects the state correctly.
144-152: The tests have been updated to check the text content of elements after changes, which is good for verifying that the UI updates as expected.
243-243: Again, the added assertion is good for ensuring that the UI reflects the state correctly after changes.
300-308: The tests have been updated to check the text content of elements after changes, which is good for verifying that the UI updates as expected.
328-393: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [319-390]
The new tests for the debounce functionality and the handling of default values are comprehensive and ensure that the new features work as expected. It's important to verify that the URL is updated correctly after the debounce timeout and that default values are handled correctly in the URL.
- 399-402: The test for default values during server-side rendering (SSR) is important to ensure that the application behaves correctly when JavaScript is disabled. This test verifies that default values are still present even without client-side navigation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/lib/sveltekit-search-params.ts (8 hunks)
Additional comments: 12
src/lib/sveltekit-search-params.ts (12)
6-12: The import of Svelte store functions and types is correctly done, ensuring that the necessary tools for state management are available.
40-40: The addition of the
showDefaultsproperty to theStoreOptionstype aligns with the PR objective to control the display of default values in the URL.53-55: The new
Overrides<T>type is introduced correctly, allowing for optional overriding of specific properties in an object of typeT.59-59: The addition of the
overridesparameter to themixSearchAndOptionsfunction is consistent with the newOverrides<T>type, but ensure that all calls to this function are updated to include this new parameter.Verification successful
The previous script failed because the
rgcommand did not recognize thetypescriptfile type. To ensure that we search all relevant files, we can omit the file type and search across all files. Let's try running the search again without specifying the file type.
The output indicates that there is only one call to
mixSearchAndOptionswithin thesrc/lib/sveltekit-search-params.tsfile. However, the output does not show the full context of the call, specifically whether the newoverridesparameter is being passed.To verify if the
overridesparameter is included in the call tomixSearchAndOptions, we need to inspect the arguments passed to the function in the provided line of code. Let's write a script to extract a few lines around the call tomixSearchAndOptionsto see the actual parameters being passed.
The provided output shows the context around the call to
mixSearchAndOptionswithin thesrc/lib/sveltekit-search-params.tsfile. It confirms that the function is called with three arguments:$page?.url?.searchParams,$overrides, andoptions. This indicates that the newoverridesparameter has been correctly included in the call tomixSearchAndOptions.Based on this information, the addition of the
overridesparameter to themixSearchAndOptionsfunction has been properly handled in the call within thesrc/lib/sveltekit-search-params.tsfile.* 170-175: The `queryParameters` function now includes the `showDefaults` property in its options, which is in line with the PR objectives. However, the default value for `showDefaults` is set to `true`, which may change the current behavior for consumers not expecting this new feature. Ensure that this change is communicated and documented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to mixSearchAndOptions to ensure they include the new overrides parameter. rg --type typescript "mixSearchAndOptions\("Length of output: 81
Script:
#!/bin/bash # Search for calls to mixSearchAndOptions to ensure they include the new overrides parameter. rg "mixSearchAndOptions\("Length of output: 126
Script:
#!/bin/bash # Extract the context around the call to mixSearchAndOptions to check the passed parameters rg -C 3 "mixSearchAndOptions\(" src/lib/sveltekit-search-params.tsLength of output: 273
177-177: The creation of a writable store for
overridesis appropriate for managing state overrides in a reactive manner.216-226: The
navigatefunction withinqueryParametersis correctly using thesortandpushHistoryoptions to control the URL navigation behavior. However, ensure that theoverrides.set({})call is intended to reset the overrides after navigation, as this could lead to unexpected behavior if overrides are meant to persist.240-247: The derived store setup is correct, and the use of
mixSearchAndOptionswithin it is appropriate. The conditional check foranyDefaultedParam && showDefaultsensures that defaults are only set when specified, which is a good implementation of the new feature.276-281: The
queryParamfunction has been updated to include theshowDefaultsproperty, which is consistent with the changes made toqueryParameters. Ensure that the default behavior change is documented.283-283: The creation of a writable store for
overrideinqueryParamis consistent with the approach taken inqueryParameters.310-320: The
navigatefunction withinqueryParamis using thesortandpushHistoryoptions correctly. The reset of theoverridestore after navigation is a potential point of confusion and should be verified for intended behavior.335-343: The derived store in
queryParamis set up correctly, with proper handling of theshowDefaultsoption. The conditional logic to set the default value only whenshowDefaultsis true is a good implementation detail.
Also feat: add
showDefaultsoption to avoid redirecting in case the search param is not presentSummary by CodeRabbit
New Features
showDefaultsoption to control the visibility of default parameter values in the URL.Enhancements
Documentation
showDefaultsoption.Tests
showDefaultsfeature.