feat: upgrade to using runes #126
Conversation
🦋 Changeset detectedLatest commit: e795e6c 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 |
WalkthroughThis pull request introduces several significant changes, including the addition of a pre-release configuration file, updates to dependencies, and modifications to Svelte components for improved management of query parameters. A new GitHub Actions workflow is included to automate package publishing. The Changes
Possibly related issues
Possibly related PRs
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for sveltekit-search-params ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (19)
playground/src/routes/default/parameters-obj/+page.svelte (2)
11-12: Consider documenting the migration from store syntax.The change from
$obj.objtoobj.objreflects the transition from stores to runes. This is a breaking change that should be documented in the pending migration guide.Would you like me to help draft documentation explaining:
- Why the store syntax ($) is no longer needed
- How to migrate existing code to the new pattern
- Benefits of using runes in this context
Line range hint
1-12: Consider adding type safety for the object structure.While the code is functional, you might want to enhance type safety for the object structure. This is especially important since runes provide better TypeScript integration.
Consider defining an interface for the object structure:
interface TestObject { test: string; } const obj = queryParameters({ obj: ssp.object<TestObject>({ test: '' }) });playground/src/routes/default/+page.svelte (2)
Line range hint
4-14: LGTM! Consider adding type safety.The consolidation of parameters using
queryParametersimproves maintainability and readability. The configuration options are properly preserved.Consider adding explicit type definitions for better type safety:
const store = queryParameters({ str2: ssp.string('str2'), -}); +} as const); const store_no_show = queryParameters( { 'str2-no-show': ssp.string('str2-no-show'), - }, + } as const, { showDefaults: false }, );
16-18: Consider consistent parameter access patterns.While the template bindings work correctly, there's an inconsistency in how parameters are accessed:
store.str2uses dot notationstore_no_show['str2-no-show']uses bracket notationConsider using bracket notation consistently for better maintainability:
-<span data-testid="str2">{store.str2}</span> +<span data-testid="str2">{store['str2']}</span> <span data-testid="str2-no-show">{store_no_show['str2-no-show']}</span>src/lib/types.ts (2)
5-7: LGTM! Consider documenting equality function behavior.The conditional type for
equalityFnis well-designed, ensuring type safety by only allowing equality comparisons for object types. This optimization can prevent unnecessary updates when values are semantically equal.Consider documenting:
- The expected behavior of the equality function (deep vs shallow comparison)
- Performance implications for large objects
- Example implementations for common use cases
10-13: LGTM! Elegant use of TypeScript's type system for currying.The intersection type elegantly combines the curried function with its base options, enabling a flexible API that supports both direct usage and partial application.
This pattern:
- Enables better composition and reusability
- Maintains type safety while providing API flexibility
- Aligns well with functional programming principles
playground/src/routes/debounce/+page.svelte (1)
Line range hint
4-11: Consider more descriptive parameter namesWhile the new
queryParametersimplementation is good, the parameter names could be more descriptive:
str2is misleading as it's a boolean parameter, not a string. Consider a name that reflects its boolean nature.Consider renaming:
const params = queryParameters( { - str2: true, + isEnabled: true, num: ssp.number(0), }, { debounceHistory: 1000 }, );playground/src/routes/equalityFn/+page.svelte (2)
2-2: Document the equality function behavior and consider type improvements.The equality function
() => falsemight not be immediately clear to other developers. Consider adding a comment explaining why it always returns false and its implications.Consider making the type more explicit:
- str: ssp.object<{ value: string }>()(() => false), + str: ssp.object<{ value: string }>()(() => false) /* Always treat as changed to force updates */,Also applies to: 4-7
17-19: Add label for better accessibility.The input field should have an associated label for better accessibility.
{#if params.str} + <label for="str-input">String Value:</label> - <input data-testid="str2-input" bind:value={params.str.value} /> + <input id="str-input" data-testid="str2-input" bind:value={params.str.value} /> <div data-testid="str2">{params.str.value}</div> {/if}.github/workflows/pkg.pr.new.yml (1)
12-16: Consider specifying exact Node.js version.While using Node.js 20.x is good, for a publishing workflow it's safer to pin to a specific version to ensure consistent builds.
- run: corepack enable - uses: actions/setup-node@v4 with: - node-version: 20.x + node-version: 20.11.0 cache: 'pnpm'playground/package.json (1)
15-16: Documentation updates needed for playgroundSince this is part of a WIP PR that mentions pending documentation, ensure the playground will include:
- Examples demonstrating the new runes syntax
- Clear indicators that this version requires Svelte 5
- Links to the migration guide (once completed)
Consider adding a comment in package.json to indicate the Svelte 5 requirement:
{ "name": "playground", "version": "0.0.1", "private": true, + "engines": { + "node": ">=16.0.0" + }, + "comments": { + "svelte": "Using Svelte 5 beta for runes support" + },src/lib/ssp/lz-encode.ts (1)
Line range hint
25-33: Consider adding input validation for decoded JSON.The current implementation catches JSON parsing errors but doesn't validate the structure of the parsed data. This could lead to runtime type errors if the decoded data doesn't match type T.
Consider adding a type guard or validation function:
function isValidData<T>(data: unknown): data is T { // Add validation logic specific to type T return true; // Replace with actual validation } // Then in the decode function: try { const parsed = JSON.parse(decompressFromEncodedURIComponent(value) ?? ''); return isValidData<T>(parsed) ? parsed : null; } catch { return null; }playground/src/routes/queryparameters/+page.svelte (1)
Documentation needed for runes-based implementation
The CHANGELOG.md shows this is version 3.0.0 with significant changes to the
queryParametersfunctionality, but there's no documentation about the runes-based implementation. Please add:
- A dedicated section in the README.md explaining the transition to runes
- Code examples showing both the old store syntax ($store) and new direct access patterns
- Documentation for the new reactive patterns using $state and $effect
- Migration steps for users upgrading from v2.x to v3.x
This will help users understand and adopt the new runes-based implementation smoothly.
🔗 Analysis chain
Line range hint
1-97: Documentation needed for runes migrationSince this is a significant change in the way state is managed (moving to runes), please ensure that the planned documentation and migration guide clearly explain:
- The transition from store syntax ($store) to direct store access
- The new reactive patterns with $state and $effect
- Best practices for handling store updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if documentation exists or is being worked on fd -e md -e mdx . | xargs rg -i "runes|migration|upgrade"Length of output: 59
Script:
#!/bin/bash # Let's check for any documentation files in common locations fd "README|CHANGELOG|MIGRATION|UPGRADE" -e md -e mdx # Also check package.json for any version changes or migration notes cat package.json | jq -r '.version, .description' # Check if there are any docs/documentation directories fd -t d -d 2 "docs|documentation" # Look for any inline documentation in the component file itself rg -i "migration|upgrade|runes" playground/src/routes/queryparameters/+page.svelteLength of output: 282
Script:
#!/bin/bash # Let's check the content of CHANGELOG.md and README.md for any migration notes cat CHANGELOG.md # Also check the playground README cat playground/README.mdLength of output: 3203
package.json (1)
44-64: Documentation update needed for dependency changesSeveral dependencies have been upgraded to their latest versions. As this PR is marked as WIP and needs documentation updates, please ensure the following in the documentation:
- Migration guide for Svelte 5 upgrade
- Any breaking changes from major version bumps
- Minimum version requirements for all dependencies
Would you like help in generating the documentation for these changes?
tests/index.test.ts (1)
362-362: Consider adding test for multiple default parametersWhile the current test is valid, consider adding a test case that verifies multiple default parameters simultaneously to ensure the behavior is consistent across different parameter types.
src/lib/ssp/json-encode.ts (2)
6-14: Avoid defaulting generic type parameters toanyfor stricter type safetyCurrently, the generic type parameters default to
any(e.g.,T extends object = any). This practice can weaken TypeScript's type checking and may allow unintended types to be used. Consider requiring explicit type parameters or providing more specific defaults to enhance type safety and catch potential bugs at compile time.Also applies to: 35-43
Line range hint
12-32: Refactor to eliminate code duplication between functionsThe implementations of
objectEncodeAndDecodeOptionsandarrayEncodeAndDecodeOptionsare nearly identical, differing primarily in the types they handle. To adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting the shared logic into a single generic function or utility that can handle both objects and arrays. This refactor would improve maintainability and reduce the risk of inconsistencies.Apply this diff to create a generic function:
+function encodeAndDecodeOptions<T>( + defaultValue?: T, +): CurriedEncodeAndDecode<T> & { defaultValue: T | undefined } { + const encode_and_decode = { + encode: (value: T) => JSON.stringify(value), + decode: (value: string | null): T | null => { + if (value === null) return null; + try { + return JSON.parse(value); + } catch { + return null; + } + }, + defaultValue, + }; + function curry_equality( + equality_fn: EncodeAndDecodeOptions<T>['equalityFn'], + ) { + return { ...encode_and_decode, equalityFn: equality_fn }; + } + return Object.assign(curry_equality, encode_and_decode); +} + -function objectEncodeAndDecodeOptions<T extends object = any>( - defaultValue?: T, -): CurriedEncodeAndDecode<T> { - // existing implementation -} - -function arrayEncodeAndDecodeOptions<T = any>( - defaultValue?: T[], -): CurriedEncodeAndDecode<T[]> { - // existing implementation -}And update any references to use
encodeAndDecodeOptionsaccordingly.Also applies to: 41-62
src/lib/sveltekit-search-params.svelte.ts (2)
10-10: Correct the grammatical error in the comment.The comment should read "This allows the application..." instead of "This allow the application..."
68-71: Remove unnecessary commented-out code.The commented-out
Overrides<T>type appears unnecessary. Removing it will improve code clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
playground/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
- .changeset/pre.json (1 hunks)
- .changeset/yellow-sloths-wave.md (1 hunks)
- .github/workflows/pkg.pr.new.yml (1 hunks)
- package.json (3 hunks)
- playground/package.json (1 hunks)
- playground/src/routes/+layout.svelte (1 hunks)
- playground/src/routes/+page.svelte (0 hunks)
- playground/src/routes/debounce/+page.svelte (2 hunks)
- playground/src/routes/default/+page.svelte (2 hunks)
- playground/src/routes/default/obj/+page.svelte (0 hunks)
- playground/src/routes/default/parameters-obj/+page.svelte (2 hunks)
- playground/src/routes/equalityFn/+page.svelte (1 hunks)
- playground/src/routes/queryparameters/+page.svelte (2 hunks)
- src/lib/index.ts (1 hunks)
- src/lib/ssp/json-encode.ts (3 hunks)
- src/lib/ssp/lz-encode.ts (2 hunks)
- src/lib/sveltekit-search-params.svelte.ts (1 hunks)
- src/lib/sveltekit-search-params.ts (0 hunks)
- src/lib/types.ts (1 hunks)
- tests/index.test.ts (5 hunks)
💤 Files with no reviewable changes (3)
- playground/src/routes/+page.svelte
- playground/src/routes/default/obj/+page.svelte
- src/lib/sveltekit-search-params.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/pre.json
- src/lib/index.ts
🔇 Additional comments (23)
playground/src/routes/+layout.svelte (2)
2-2: LGTM! Clean migration to Svelte 5 runesThe script block successfully migrates to Svelte 5 runes syntax. The
$effectreplacement foronMountis appropriate, and the data attribute initialization is preserved.Also applies to: 4-5
9-9: LGTM! Proper slot migration patternThe template correctly migrates from
<slot />to the new runes pattern using{@render children?.()}. The optional chaining operator provides good defensive programming.playground/src/routes/default/parameters-obj/+page.svelte (2)
1-2: LGTM! Correct runes setup.The
<svelte:options runes />declaration is correctly placed at the top of the file, which is required for using the new runes syntax.
Line range hint
3-6: Verify the imported functions support runes.The transition to using
queryParametersinstead of store-based approach aligns with the runes upgrade. However, we should verify that the imported functions from 'sveltekit-search-params' are designed to work with runes.✅ Verification successful
The
queryParametersfunction is compatible with runesThe implementation shows full compatibility with runes:
- Uses
$state,$derived, and$effectrunes in its implementation- The playground examples demonstrate successful usage with
<svelte:options runes />- The function properly handles reactive state management and updates using the new runes system
- All example files in the playground successfully use
queryParameterswith runes enabledThe transition to using
queryParametersis appropriate and fully supported in the runes-based architecture.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if queryParameters is properly exported and documented for runes rg -A 5 "export.*queryParameters" rg -A 5 "queryParameters.*runes"Length of output: 627
Script:
#!/bin/bash # Let's check the implementation of queryParameters to understand if it's compatible with runes ast-grep --pattern 'function queryParameters<$_>($_) { $$$ }' # Also check for any runes-related changes or documentation rg -l "runes"Length of output: 224
Script:
#!/bin/bash # Let's examine the content of these files to understand the runes usage context cat playground/src/routes/default/+page.svelte playground/src/routes/default/parameters-obj/+page.svelte playground/src/routes/debounce/+page.svelte # Also check the implementation details of the queryParameters function cat src/lib/sveltekit-search-params.svelte.tsLength of output: 10065
src/lib/types.ts (1)
15-15: Verify impact of removing generic type parameter.The simplification of
StoreOptionsto remove the generic type parameter makes sense as these options are type-agnostic. However, we should verify this change doesn't break existing code.✅ Verification successful
Removal of generic type parameter from StoreOptions is safe
The search results confirm that
StoreOptionsis used consistently throughout the codebase without any generic type parameters. All usages are type-agnostic and only deal with behavioral options likedebounceHistory,pushHistory, andsort. The type is primarily used as a parameter in function signatures and is imported/exported without any type parameters, indicating the removal of the generic type is safe and aligns with its actual usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of StoreOptions with generic type parameters # that might break with this change # Look for StoreOptions<...> pattern in TypeScript files rg "StoreOptions<" -t ts # Look for potential type assertions or extensions ast-grep --pattern 'type $_ = StoreOptions<$_>'Length of output: 76
Script:
#!/bin/bash # Let's try a broader search to understand the usage of StoreOptions # Find all references to StoreOptions rg "StoreOptions" -A 3 -B 3 -t ts # Find type declarations that might extend or use StoreOptions ast-grep --pattern 'type $_ = $$$StoreOptions$$$' # Find interface declarations that might extend StoreOptions ast-grep --pattern 'interface $_ extends StoreOptions' # Find variable declarations using StoreOptions type ast-grep --pattern 'const $_: StoreOptions = $_'Length of output: 3946
playground/src/routes/debounce/+page.svelte (4)
1-1: LGTM: Runes configuration properly enabledThe
<svelte:options runes />directive is correctly placed at the top of the file, enabling the new runes syntax as intended by the PR.
14-15: LGTM: Clean input binding and display implementationThe input binding and display implementation are correctly updated to use the new params object while maintaining test IDs.
19-21: LGTM: Clean button handler implementationThe button handler correctly implements the increment operation using the new params object.
Line range hint
3-11: Verify complete migration to runes syntaxLet's verify that all reactive statements in this file are using the new runes syntax and no old-style reactivity remains.
✅ Verification successful
Runes migration is properly implemented
The file is correctly using runes syntax:
- Has
<svelte:options runes />declaration at the top- No old-style reactive statements (
$:) are present- No store subscriptions using
$storesyntax- All reactivity is handled through the
queryParametersAPI🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old-style reactive statements ($: or $store syntax) rg -l '\$:|\$[a-zA-Z]' playground/src/routes/debounce/+page.svelteLength of output: 68
Script:
#!/bin/bash # Let's first check the entire content of the file cat playground/src/routes/debounce/+page.svelte # Also search for reactive declarations with context rg -A 2 -B 2 '\$:|\$[a-zA-Z]' playground/src/routes/debounce/+page.svelteLength of output: 675
playground/src/routes/equalityFn/+page.svelte (2)
9-9: LGTM! Clean state declaration using runes.The state declaration is clear and follows the new runes syntax.
1-19: Verify runes migration completeness.Since this is part of the runes upgrade, let's verify that all reactive declarations have been migrated to the new syntax.
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to ensure we catch all potential cases that need migration to runes, including store subscriptions and reactive declarations.
Migration to runes appears complete in this file
The verification shows that all reactive syntax in the file is using the new runes pattern:
- Using
$state()for state management- Using
$effect()for reactive declarations- No old-style
$:reactive declarations found- Store subscriptions are properly handled with the new syntax
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining old-style reactive declarations in the playground rg '\$:' playground/src/routesLength of output: 32
Script:
#!/bin/bash # Let's search for reactive declarations using ast-grep to ensure we catch all patterns ast-grep --pattern '$: $_' # Also search for any $-prefixed variables which might indicate old store syntax rg '\$[a-zA-Z]' playground/src/routes # Search for let declarations that might need migration ast-grep --pattern 'let $_ = $_'Length of output: 2172
.github/workflows/pkg.pr.new.yml (1)
1-24: Verify the custom publishing tool configuration.The workflow uses
pkg-pr-newwhich appears to be a custom publishing tool. Since this PR is marked as WIP and involves upgrading to runes, we should ensure this tool is compatible with the new changes.Let's check for any configuration files or documentation for this tool:
playground/package.json (1)
15-16: Major version upgrades require careful considerationThe upgrade to Svelte 5 (from 4.x) and @sveltejs/vite-plugin-svelte 4.0.0 (from 3.x) represents significant changes:
- Svelte 5 introduces the new runes API which is currently in beta
- These changes might introduce breaking changes in the playground
Consider the following recommendations:
- Since this is a major version upgrade, ensure all playground examples are updated to demonstrate both traditional and runes-based approaches
- Add a note in the playground README about the Svelte 5 beta status
- Consider maintaining a separate branch with Svelte 4 compatibility until Svelte 5 is stable
✅ Verification successful
Svelte 5 upgrade is compatible with existing code
The codebase inspection shows that the playground files are already using Svelte 5 runes syntax and features:
- Files use
$stateand$effectrunes- Explicit runes opt-in with
<svelte:options runes />- Proper usage of
untrackfrom SvelteThe upgrade to Svelte 5 and vite-plugin-svelte 4.0.0 is appropriate as the playground code is specifically designed to demonstrate and test the new runes API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any Svelte-specific warnings or deprecation notices in the codebase # that might need attention with the upgrade # Look for potentially affected Svelte syntax rg -g '*.svelte' '\$:|\{#each|\{#if|\{#await' -l # Look for reactive statements that might need migration to runes ast-grep --pattern 'let $name = $_'Length of output: 243
Script:
#!/bin/bash # Let's examine the specific Svelte syntax in the affected files # and check for patterns that might need updates with Svelte 5 # Check queryparameters page echo "=== queryparameters/+page.svelte ===" cat playground/src/routes/queryparameters/+page.svelte echo -e "\n=== equalityFn/+page.svelte ===" cat playground/src/routes/equalityFn/+page.svelte echo -e "\n=== parameters-obj/+page.svelte ===" cat playground/src/routes/default/parameters-obj/+page.svelte # Also check if there's any documentation or migration notes echo -e "\n=== README or documentation ===" fd -g "README*" -x cat {}Length of output: 20019
src/lib/ssp/lz-encode.ts (1)
1-4: LGTM! Import changes align with the new curried function pattern.playground/src/routes/queryparameters/+page.svelte (1)
2-3: LGTM: Import changes align with runes upgradeThe addition of
untrackfrom Svelte is appropriate for optimizing reactivity in the new runes-based implementation.package.json (3)
8-8: LGTM: Package manager specification addedAdding
"packageManager": "pnpm@9.9.0"is a good practice as it:
- Ensures consistent package management across development environments
- Aligns with the new GitHub Actions workflow mentioned in the PR summary
Line range hint
69-71: Verify peer dependency compatibilityThe peer dependencies have been correctly updated to support Svelte 5:
"peerDependencies": { "@sveltejs/kit": "^1.0.0 || ^2.0.0", "svelte": "^3.55.0 || ^4.0.0 || ^5.0.0" }Let's verify the compatibility:
#!/bin/bash # Check for any version-specific imports or syntax that might need updates rg -g '!node_modules' -g '!*.json' 'from ["'\''](svelte|@sveltejs\/kit)' -A 2
56-56: Major architectural change: Upgrade to Svelte 5The upgrade to Svelte 5 (
"svelte": "^5.1.3") introduces significant changes, particularly the new runes system. This aligns with the PR objective but requires careful consideration:
- This is a breaking change that will require updates to the codebase
- Users will need migration guidance
Let's verify the impact:
tests/index.test.ts (2)
326-326: LGTM: URL parameter ordering assertions are consistentThe modified assertions correctly verify the expected URL parameter ordering behavior.
Also applies to: 344-344, 352-352
460-469: Verify the reduced number of store changesThe test now expects 3 store changes instead of 4, and uses clipboard operations. Please verify:
- Why the number of store changes decreased
- Document the expected triggers for store changes
- Explain why clipboard operations are now required
Let me help verify the changes:
#!/bin/bash # Search for related store change implementations rg -A 5 'how-many-store-changes' src/src/lib/ssp/json-encode.ts (2)
27-32: Verify the use ofObject.assignwith function objectsUsing
Object.assign(curry_equality, encode_and_decode)merges the properties ofencode_and_decodeinto thecurry_equalityfunction object. While this is a clever approach to combine functionality, it's important to ensure that this pattern behaves as expected and doesn't introduce any unexpected side effects, especially regarding type safety and property enumeration in the resulting object.Also applies to: 56-61
1-4: LGTM!The imports are correctly defined, and the necessary types are imported without issues.
src/lib/sveltekit-search-params.svelte.ts (1)
80-81:⚠️ Potential issueVerify the import of
fromStorefromsvelte/store.The
fromStorefunction is imported fromsvelte/store, but it may not be part of the official Svelte API. Ensure thatfromStoreis correctly imported or defined.Run the following script to verify if
fromStoreis exported:✅ Verification successful
fromStoreis a Svelte 5 API featureLooking at the package.json, this project is using Svelte 5 (
"svelte": "^5.1.3") which introduces thefromStorefunction as part of its new reactivity system. The import and usage are correct for Svelte 5.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'fromStore' is exported from 'svelte/store' # Test: Search for 'fromStore' in the svelte package. Expectation: 'fromStore' should be defined. rg 'export.*fromStore' node_modules/svelteLength of output: 104
Script:
#!/bin/bash # Let's check the imports in the file and search for fromStore definition rg "import.*fromStore" -A 2 # Also search for any local definition of fromStore ast-grep --pattern 'function fromStore' # Check if fromStore is exported from any local file rg "export.*fromStore" # Let's also see the actual import statement in the file cat src/lib/sveltekit-search-params.svelte.tsLength of output: 9056
Script:
#!/bin/bash # Let's check the package.json to see if this is using the latest Svelte version cat package.json # Let's also check if there's any local implementation of fromStore rg -g '!node_modules' -g '!dist' "fromStore" -B 2 -A 2Length of output: 3750
package.json
Outdated
| @@ -94,6 +94,6 @@ | |||
| } | |||
| }, | |||
| "dependencies": { | |||
| "@sveltejs/vite-plugin-svelte": "^3.0.2" | |||
| "@sveltejs/vite-plugin-svelte": "^4.0.0" | |||
There was a problem hiding this comment.
why is vps a dependency here, should be a peerDependency if its really needed, but i thought this was runtime only?
There was a problem hiding this comment.
It's definitely a mistake...should've always been in devDeps (possibly in the peers). I don't think i ever touched this so it's probably a byproduct of me generating this lib project a long time ago. Let me move it to devDeps
There was a problem hiding this comment.
@dominikg i didn't add @sveltejs/vite-plugin-svelte because it already has a peerdep on kit and svelte...do you think it should also be a peerdep (maybe specifying ^4.0.0?
There was a problem hiding this comment.
your package does not depend on vps itself, does it? the peer dependency on sveltekit is because it uses its page store but you don't have to repeat a transitive peer dep unless you need it yourself
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/index.test.ts (1)
460-469: Document the expected behavior in equalityFn test
While the test modifications look good, please add comments explaining:
- Why clipboard operations are used in this test
- What triggers the 3 store changes
- How this relates to the equalityFn functionality
Example:
test('equalityFn impact the amount of rerenders with queryParameters', async ({
page,
context,
}) => {
+ // Using clipboard operations to simulate user input that should trigger store changes:
+ // 1. Initial load
+ // 2. Copy operation
+ // 3. Paste operation
await context.grantPermissions(['clipboard-read', 'clipboard-write']);
await page.goto('/equalityFn?str=%7B%22value%22%3A%22test%22%7D');📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
- .github/workflows/pkg.pr.new.yml (1 hunks)
- package.json (1 hunks)
- tests/index.test.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pkg.pr.new.yml
- package.json
🔇 Additional comments (3)
tests/index.test.ts (3)
4-4: Skipping queryParam test suite requires documentation
The previous review comment about premature test skipping is still valid. Since this PR is upgrading to runes, please ensure the following before skipping tests:
- Document the deprecation plan in CHANGELOG.md
- Provide migration steps in the planned migration guide
- Update documentation to reflect the changes
326-326: LGTM: URL search parameter assertions
The modifications to URL search parameter assertions maintain the same test logic while updating the expected values appropriately.
Also applies to: 344-344, 352-352, 362-362
Line range hint 372-384: Remove redundant skipped test
As noted in the previous review, this test can be safely removed since the same functionality is covered by the queryParameters test below.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/lib/sveltekit-search-params.svelte.ts (1)
272-363: Documentation needed for runes implementation.
As mentioned in the PR objectives, documentation updates are needed. Please include:
- Migration guide for users moving from the old implementation
- Examples of using the new runes-based API
- Explanation of the reactive behavior with $state, $derived, and $effect
Would you like help in drafting the documentation and migration guide?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/lib/sveltekit-search-params.svelte.ts (1 hunks)
🔇 Additional comments (3)
src/lib/sveltekit-search-params.svelte.ts (3)
1-21: LGTM: Clean initialization with proper build-time handling.
The setup properly handles both browser and build environments, with appropriate use of example.com as the default URL.
332-346: Verify effect cleanup.
The pre-effect subscription might need cleanup to prevent memory leaks. Consider whether this effect needs to be cleaned up when parameters are removed or the component is destroyed.
#!/bin/bash
# Search for effect cleanup patterns
rg -A 5 '\$effect\.pre.*\{|\$effect\.root.*\{|\$effect.*cleanup'89-150: Verify batch update cleanup on navigation interruption.
The batched updates mechanism might leave stale entries if navigation is interrupted or fails. Consider adding cleanup in an error handler and when the component is destroyed.
This is still a WIP because i need to update the docs but i wanted to put it out to get some feedback from the users. I will also need to provide a migration guide.
Summary by CodeRabbit
Release Notes
New Features
sveltekit-search-paramspackage.sveltekit-search-paramspackage to manage search parameters more effectively, including the introduction of a recursive proxy for dynamic access.Bug Fixes
Documentation
Chores