Add ssp for base64 string encoding #117
Conversation
I was looking to offer another ssp definition but I was worried the main file was getting a bit too large, so I split the existing definitions out first.
|
WalkthroughThe pull request includes several changes across multiple files, primarily focusing on enhancing the handling of base64 encoded data within a Svelte application. Key modifications include updates to the ESLint configuration, enhancements to the Svelte components for managing base64 parameters, and the introduction of new encoding and decoding functions. Additionally, the README file has been corrected for clarity, and various types have been added to improve type safety. The changes aim to streamline functionality while ensuring that existing features remain intact. Changes
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
src/lib/types.ts (3)
1-5: LGTM! Consider enhancing type safety forencodeanddecode.The
EncodeAndDecodeOptions<T>type definition is well-structured and aligns with the PR objective of adding support for base64 string encoding. It provides flexibility through the generic type parameterTand includes appropriate function signatures for encoding and decoding operations.To further enhance type safety, consider using
NonNullable<T>for theencodefunction parameter:export type EncodeAndDecodeOptions<T = any> = { - encode: (value: T) => string | undefined; + encode: (value: NonNullable<T>) => string | undefined; decode: (value: string | null) => T | null; defaultValue?: T; };This change ensures that
nullorundefinedvalues cannot be passed to theencodefunction, potentially preventing runtime errors.
7-15: LGTM! Consider adding JSDoc comments for better documentation.The
StoreOptions<T>type definition is well-structured and provides a good balance of flexibility and type safety. The use of optional properties and the conditional type forequalityFnare particularly noteworthy.To improve clarity and maintainability, consider adding JSDoc comments to describe the purpose of each property:
export type StoreOptions<T> = { /** Delay in milliseconds for debouncing history updates */ debounceHistory?: number; /** Whether to push updates to browser history */ pushHistory?: boolean; /** Whether to sort the parameters */ sort?: boolean; /** Whether to show default values */ showDefaults?: boolean; /** * Custom equality function for comparing values * Only available when T extends object */ equalityFn?: T extends object ? (current: T | null, next: T | null) => boolean : never; };These comments will provide valuable context for developers using this type in the future.
1-15: Overall, excellent additions to support base64 string encoding.The new type definitions
EncodeAndDecodeOptions<T>andStoreOptions<T>are well-designed and provide a solid foundation for implementing base64 string encoding support. They offer a good balance of flexibility, type safety, and configurability.As you continue to develop this feature:
- Ensure that the implementations using these types handle edge cases, such as encoding/decoding errors.
- Consider adding unit tests to verify the behavior of functions that will use these types.
- Update any relevant documentation to reflect these new options and their usage.
src/lib/ssp/util.ts (1)
3-18: Function implementation looks good, with some suggestions for improvement.The
primitiveEncodeAndDecodeOptionsfunction is well-implemented. Here are some suggestions to enhance it:
- Consider adding JSDoc comments to explain the purpose of the function and its parameters.
- You might want to add type assertions or runtime checks to ensure that
encodeanddecodeare actually functions.- Consider adding a runtime check in the
sspfunction to ensure thatdefaultValue(if provided) is of typeT.Here's an example of how you could implement these suggestions:
/** * Creates a function for encoding and decoding primitive values with an optional default value. * @template T The type of the value to be encoded/decoded * @param {Object} options - The encoding and decoding functions * @param {(value: T) => string | undefined} options.encode - Function to encode a value * @param {(value: string | null) => T | null} options.decode - Function to decode a value * @returns A function that can be called with or without a default value */ export function primitiveEncodeAndDecodeOptions<T>({ encode, decode, }: { encode: (value: T) => string | undefined; decode: (value: string | null) => T | null; }) { if (typeof encode !== 'function' || typeof decode !== 'function') { throw new Error('encode and decode must be functions'); } function ssp( defaultValue: T, ): EncodeAndDecodeOptions<T> & { defaultValue: T }; function ssp(): EncodeAndDecodeOptions<T> & { defaultValue: undefined }; function ssp(defaultValue?: T): EncodeAndDecodeOptions<T> { if (arguments.length > 0 && typeof defaultValue === 'undefined') { throw new Error('defaultValue must be of type T when provided'); } return { encode, decode, defaultValue }; } return ssp; }These changes improve the function's documentation, type safety, and runtime error handling.
src/lib/ssp/base64.ts (1)
1-37: Overall implementation is solid, with room for minor improvements.The
sspBase64function provides a robust implementation for encoding and decoding Unicode strings as UTF-8 Base64 URL-safe strings. It handles edge cases well and uses modern Web APIs. The function signature and return type are well-defined, providing good TypeScript support.While the current implementation is functional, consider the optimizations suggested in the previous comments for both the
encodeanddecodemethods to improve readability and potentially performance.As a minor addition, consider adding input validation for the
decodemethod to ensure the input string is a valid Base64 URL-safe string. This could prevent potential errors when decoding invalid inputs. For example:decode: (value: string | null) => { if (value === null) return ''; if (!/^[A-Za-z0-9\-_]*$/.test(value)) { throw new Error('Invalid Base64 URL-safe string'); } // ... rest of the decode implementation },This addition would make the function more robust against potential misuse.
playground/src/routes/queryparameters/+page.svelte (3)
11-11: LGTM! Consider adding a comment for clarity.The addition of the
base64parameter usingssp.base64()is correct and aligns with the PR objective.Consider adding a brief comment explaining the purpose of this parameter for improved code readability:
const store = queryParameters({ str: true, num: ssp.number(), bools: ssp.boolean(), obj: ssp.object<{ str: string }>(), arr: ssp.array<number>(), lz: ssp.lz<string>(), + // Handle base64-encoded values in query parameters base64: ssp.base64(), });
89-89: LGTM! Consider wrapping the value in curly braces for consistency.The display element for the
base64parameter is implemented correctly. For consistency with other similar elements in this file, consider wrapping the value in curly braces:-<div data-testid="base64">{$store.base64}</div> +<div data-testid="base64">{JSON.stringify($store.base64)}</div>This change would make it consistent with how the
objparameter is displayed (line 45).
Unexpected usage of
base64parameter inplayground/src/routes/+page.svelteThe
base64parameter is found inplayground/src/routes/+page.svelte, which was not part of the reviewed changes. Please verify whether this usage is intentional and ensure it doesn't conflict with the intended functionality inplayground/src/routes/queryparameters/+page.svelte.
playground/src/routes/+page.svelte:
- Line 1:
const base64 = queryParam('base64', ssp.base64());- Line 2:
let base64_changes = 0;- Line 3:
$base64;- Line 4:
base64_changes++;- Line 5:
<input data-testid="base64-input" bind:value={$base64} />- Line 6:
<div data-testid="base64">{$base64}</div>- Line 7:
<p data-testid="how-many-base64-changes">{base64_changes}</p>🔗 Analysis chain
Line range hint
1-90: Overall, the implementation looks good. Verify the new base64 functionality.The changes to add support for base64 string encoding have been implemented correctly and integrate well with the existing code. The new
base64parameter is added to the store, and appropriate UI elements for input and display have been included.To ensure the new functionality works as expected, please verify:
- The base64 encoding/decoding is working correctly.
- The URL updates appropriately when the base64 input changes.
- The state is correctly restored when loading a URL with a base64 parameter.
You can use the following script to check for any unexpected uses of the new
base64parameter:This will help ensure that the new feature is isolated to the intended file and hasn't inadvertently affected other parts of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any unexpected uses of the new base64 parameter # Test: Search for uses of 'base64' outside the reviewed file rg --type svelte 'base64' --glob '!playground/src/routes/queryparameters/+page.svelte'Length of output: 117
Script:
#!/bin/bash # Description: Check for any unexpected uses of the new base64 parameter # Test: Search for uses of 'base64' in .svelte files excluding the reviewed file rg 'base64' -g '*.svelte' -g '!playground/src/routes/queryparameters/+page.svelte'Length of output: 597
playground/src/routes/+page.svelte (3)
13-13: LGTM! Consider adding a comment for clarity.The addition of the
base64query parameter usingssp.base64()is well-implemented and aligns with the PR objective.Consider adding a brief comment explaining the purpose of this parameter for improved code readability:
+// Query parameter for handling base64 encoded strings const base64 = queryParam('base64', ssp.base64());
36-40: LGTM! Consider adding a comment for consistency.The reactive statement for tracking changes to
$base64is well-implemented and consistent with the existing pattern.For consistency with other similar blocks, consider adding a comment explaining the purpose of this reactive statement:
$: { + // Increment base64_changes when $base64 changes // eslint-disable-next-line @typescript-eslint/no-unused-expressions $base64; base64_changes++; }
97-98: LGTM! Consider grouping related elements.The additions for the base64 input, display, and changes count are well-implemented and consistent with the existing pattern.
For better code organization, consider grouping the base64-related elements together:
<input data-testid="base64-input" bind:value={$base64} /> <div data-testid="base64">{$base64}</div> <p data-testid="how-many-base64-changes">{base64_changes}</p>This grouping would make it easier to understand and maintain the base64-related functionality.
Also applies to: 111-111
tests/index.test.ts (3)
109-117: LGTM! Consider adding a test for decoding.The test case for base64 encoding is well-structured and covers a good range of characters, including Unicode. It correctly verifies both the displayed text and the URL parameter.
Consider adding a test case for decoding the base64 string back to the original input to ensure round-trip consistency.
119-127: LGTM! Consider adding a comment explaining the expected number of changes.This test case effectively checks that modifying the base64 input doesn't cause excessive reactivity triggers. It's concise and focuses on an important aspect of performance.
Consider adding a brief comment explaining why the expected number of changes is 2. This would improve the test's readability and make it easier for other developers to understand the expected behavior.
285-293: LGTM! Consider refactoring to reduce duplication.This test case correctly verifies the base64 encoding functionality for the 'queryParameters' suite, similar to the test in the 'queryParam' suite.
Consider creating a shared test utility function for the base64 encoding test, which can be used by both 'queryParam' and 'queryParameters' suites. This would reduce code duplication and make it easier to maintain these tests in the future.
Example:
function testBase64Encoding(page: Page, inputSelector: string, outputSelector: string) { // Common test logic here } // In queryParam suite test('works as expected with base64', async ({ page }) => { await testBase64Encoding(page, '[data-testid="base64-input"]', '[data-testid="base64"]'); }); // In queryParameters suite test('works as expected with base64', async ({ page }) => { await testBase64Encoding(page, '[data-testid="base64-input"]', '[data-testid="base64"]'); });src/lib/ssp/json-encode.ts (1)
13-21: Consider logging or handling errors when JSON parsing fails indecodefunctionsIn both
decodefunctions at lines 14-21 and 37-44, whenJSON.parsethrows an error during parsing, the exception is caught silently, andnullis returned. Suppressing errors can make debugging difficult if unexpected input causes failures.Consider logging the error or providing additional context to aid in debugging. For example, you could log the error message or include a comment explaining why the error is suppressed.
Also applies to: 36-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- README.md (7 hunks)
- eslint.config.js (1 hunks)
- playground/src/routes/+page.svelte (4 hunks)
- playground/src/routes/queryparameters/+page.svelte (2 hunks)
- src/lib/index.ts (1 hunks)
- src/lib/ssp/base64.ts (1 hunks)
- src/lib/ssp/index.ts (1 hunks)
- src/lib/ssp/json-encode.ts (1 hunks)
- src/lib/ssp/lz-encode.ts (1 hunks)
- src/lib/ssp/util.ts (1 hunks)
- src/lib/sveltekit-search-params.ts (2 hunks)
- src/lib/types.ts (1 hunks)
- tests/index.test.ts (2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~57-~57: Possible missing comma found.
Context: ...{$username} ``` the function returns a store so make sure to use it with the$pre...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~61-~61: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...meter) Reading query parameters is cool but you know what is even cooler? Writing q...(COMMA_COMPOUND_SENTENCE)
[typographical] ~61-~61: It appears that a comma is missing.
Context: ...er? Writing query parameters! With this library you can treat your store just like norm...(DURING_THAT_TIME_COMMA)
[uncategorized] ~61-~61: Possible missing comma found.
Context: ...o update the state and consequently the url you can just do this ```svelte <script...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~110-~110: Possible missing comma found.
Context: ...ould be of type number and the decoding function it's what's used to update the url when...(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (17)
src/lib/index.ts (3)
2-2: LGTM: Import statement forsspadded correctly.The import statement for
sspfrom the './ssp' module has been added correctly. This is necessary for the subsequent export ofssp.
4-4: LGTM: File extension removed from import path.The removal of the '.js' file extension from the import path is a good practice. This change aligns with modern JavaScript/TypeScript module resolution strategies and improves compatibility across different build tools and environments.
5-5: LGTM: Export statement forsspadded correctly.The export statement for
ssphas been added correctly, corresponding to the import statement on line 2. This change makes thesspfunctionality available to consumers of this module.src/lib/ssp/util.ts (2)
1-1: LGTM: Import statement is correct.The import of
EncodeAndDecodeOptionstype from '$lib/types' is appropriate and necessary for the function implementation.
1-18: Overall, the implementation looks good and aligns with the PR objectives.The
primitiveEncodeAndDecodeOptionsfunction provides a flexible way to create encoding and decoding options for primitive values, which can be used for base64 string encoding as mentioned in the PR objectives. The function is well-structured and type-safe.To further improve the implementation:
- Consider adding unit tests to verify the behavior of the
primitiveEncodeAndDecodeOptionsfunction and its returnedsspfunction.- Ensure that this implementation is compatible with the changes in PR chore: split out the ssp definitions #116, as mentioned in the PR description.
- Update the documentation (if any) to reflect these new capabilities for encoding and decoding options.
To verify the usage and impact of this new function, you can run the following script:
This script will help identify any immediate usage of the new function and potential conflicts with PR #116.
✅ Verification successful
Verification Successful: No Issues Found
The usage of
primitiveEncodeAndDecodeOptionshas been confirmed in the following files:
src/lib/ssp/util.tssrc/lib/ssp/index.tsThere are no conflicts with PR #116 beyond the expected changes in
src/lib/ssp/util.ts.Next Steps:
- Unit Tests: Add unit tests to verify the behavior of the
primitiveEncodeAndDecodeOptionsfunction and its returnedsspfunction across all usage points.- Compatibility Check: Ensure that the implementation is compatible with other changes in PR chore: split out the ssp definitions #116.
- Documentation: Update the documentation to reflect the new capabilities for encoding and decoding options.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of primitiveEncodeAndDecodeOptions and potential conflicts with PR #116 # Test 1: Check for usage of primitiveEncodeAndDecodeOptions echo "Checking for usage of primitiveEncodeAndDecodeOptions:" rg --type typescript "primitiveEncodeAndDecodeOptions" # Test 2: Check for potential conflicts with PR #116 echo "Checking for potential conflicts with PR #116:" gh pr diff 116 | rg "src/lib/ssp/util.ts"Length of output: 434
Script:
#!/bin/bash # Description: Corrected script to check for usage of primitiveEncodeAndDecodeOptions and potential conflicts with PR #116 # Test 1: Check for usage of primitiveEncodeAndDecodeOptions echo "Checking for usage of primitiveEncodeAndDecodeOptions:" rg --type ts "primitiveEncodeAndDecodeOptions" # Test 2: Check for potential conflicts with PR #116 echo "Checking for potential conflicts with PR #116:" gh pr diff 116 | rg "src/lib/ssp/util.ts"Length of output: 736
src/lib/ssp/index.ts (3)
1-7: LGTM: Imports are well-structured and purposeful.The imports are correctly organized, bringing in specific encoding and decoding functions from separate modules. This structure promotes modularity and maintainability.
9-26: LGTM: Well-structured and type-safe exported object.The default export provides a clear and concise way to access all encoding/decoding options. The use of
as constensures type safety and immutability, which is a good practice.
22-25: LGTM: Good use of modular imported functions.The use of imported functions for complex types (object, array) and special encodings (lz, base64) promotes code reuse and modularity. This structure keeps the main file clean and easy to understand.
To ensure the correctness of these imported functions, please run the following verification script:
This script will help verify the implementation of the imported encoding/decoding functions in their respective files.
✅ Verification successful
Verified: Exported encoding/decoding functions are correctly implemented and properly imported.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of imported encoding/decoding functions # Test: Check the implementation of objectEncodeAndDecodeOptions and arrayEncodeAndDecodeOptions echo "Checking json-encode.ts implementation:" rg --type typescript -A 10 'export const (object|array)EncodeAndDecodeOptions' src/lib/ssp/json-encode.ts # Test: Check the implementation of lzEncodeAndDecodeOptions echo "Checking lz-encode.ts implementation:" rg --type typescript -A 10 'export const lzEncodeAndDecodeOptions' src/lib/ssp/lz-encode.ts # Test: Check the implementation of sspBase64 echo "Checking base64.ts implementation:" rg --type typescript -A 10 'export const sspBase64' src/lib/ssp/base64.tsLength of output: 635
Script:
#!/bin/bash # Description: Verify the implementation of imported encoding/decoding functions # Test: Check the implementation of objectEncodeAndDecodeOptions and arrayEncodeAndDecodeOptions echo "Checking json-encode.ts implementation:" rg -g "*.ts" -A 10 'export const (objectEncodeAndDecodeOptions|arrayEncodeAndDecodeOptions)' src/lib/ssp/json-encode.ts # Test: Check the implementation of lzEncodeAndDecodeOptions echo "Checking lz-encode.ts implementation:" rg -g "*.ts" -A 10 'export const lzEncodeAndDecodeOptions' src/lib/ssp/lz-encode.ts # Test: Check the implementation of sspBase64 echo "Checking base64.ts implementation:" rg -g "*.ts" -A 10 'export const sspBase64' src/lib/ssp/base64.tsLength of output: 528
Script:
#!/bin/bash # Description: List all export statements in the target TypeScript files and search for the specific constants. # Check if the target files exist for file in src/lib/ssp/json-encode.ts src/lib/ssp/lz-encode.ts src/lib/ssp/base64.ts; do if [ -f "$file" ]; then echo "Listing all export statements in $file:" rg -g "*.ts" '^export' "$file" echo "" else echo "File not found: $file" echo "" fi done # Search for the specific constants with a broader pattern echo "Searching for the constants with a broader pattern:" rg -g "*.ts" 'objectEncodeAndDecodeOptions|arrayEncodeAndDecodeOptions|lzEncodeAndDecodeOptions|sspBase64' src/lib/ssp/Length of output: 2939
Script:
#!/bin/bash # Description: Verify the implementation of imported encoding/decoding functions by searching for exported functions. # Check if the target files exist and list all exported functions for file in src/lib/ssp/json-encode.ts src/lib/ssp/lz-encode.ts src/lib/ssp/base64.ts; do if [ -f "$file" ]; then echo "Listing all exported functions in $file:" rg -g "*.ts" '^export function' "$file" echo "" else echo "File not found: $file" echo "" fi done # Search for the specific exported functions echo "Searching for the specific exported functions:" rg -g "*.ts" 'export function (objectEncodeAndDecodeOptions|arrayEncodeAndDecodeOptions|lzEncodeAndDecodeOptions|sspBase64)' src/lib/ssp/Length of output: 1576
Script:
#!/bin/bash # Description: Verify the implementation of imported encoding/decoding functions by searching for specific export statements. # Check if the target files exist and list all 'export { ... }' statements for file in src/lib/ssp/json-encode.ts src/lib/ssp/base64.ts; do if [ -f "$file" ]; then echo "Listing all 'export { ... }' statements in $file:" rg -g "*.ts" '^export\s*\{' "$file" echo "" else echo "File not found: $file" echo "" fi done # Search for the specific exported constants within 'export { ... }' statements echo "Searching for specific exported constants within 'export { ... }' statements:" rg -g "*.ts" 'export\s*\{[^}]*(objectEncodeAndDecodeOptions|arrayEncodeAndDecodeOptions|sspBase64)[^}]*\}' src/lib/ssp/Length of output: 1161
eslint.config.js (1)
52-52: Consider the implications of disabling the 'no-explicit-any' rule.The addition of
'@typescript-eslint/no-explicit-any': 'off'disables the TypeScript ESLint check for explicit 'any' types. While this can be necessary in certain scenarios, it's important to consider the following:
- This change allows the use of 'any' type without triggering linter errors, which might lead to reduced type safety in the codebase.
- It's generally recommended to use more specific types when possible to leverage TypeScript's full potential.
Could you please clarify:
- Why is this rule being disabled globally?
- Are there specific parts of the codebase that require the use of 'any' type?
- Have you considered using more specific types or the 'unknown' type as an alternative?
To help verify the necessity of this change, you can run the following script to check for current usage of 'any' in the codebase:
This will help understand the current usage and potential impact of disabling this rule.
✅ Verification successful
The 'no-explicit-any' rule has no current impact.
No existing code uses the
anytype, so disabling the'@typescript-eslint/no-explicit-any'rule does not affect the current codebase. This change can be safely merged.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of 'any' type in TypeScript files # Test: Search for 'any' type usage echo "Files using 'any' type:" rg --type typescript --type svelte ': any' -l # Test: Count occurrences of 'any' type echo "Total occurrences of 'any' type:" rg --type typescript --type svelte ': any' --count-matches | awk -F':' '{sum += $2} END {print sum}'Length of output: 363
Script:
#!/bin/bash # Description: Check for usage of 'any' type in TypeScript and Svelte files # Test: Search for 'any' type usage with correct file types echo "Files using 'any' type:" rg --type ts --type svelte 'any' -l # Test: Count occurrences of 'any' type with correct file types echo "Total occurrences of 'any' type:" rg --type ts --type svelte 'any' --count-matches | awk -F':' '{sum += $2} END {print sum}'Length of output: 331
Script:
#!/bin/bash # Description: Check for usage of 'any' type in TypeScript (.ts, .tsx) and Svelte (.svelte) files # Test: Search for 'any' type usage in specified file extensions echo "Files using 'any' type:" rg 'any' --glob '*.ts' --glob '*.tsx' --glob '*.svelte' -l # Test: Count occurrences of 'any' type in specified file extensions echo "Total occurrences of 'any' type:" rg 'any' --glob '*.ts' --glob '*.tsx' --glob '*.svelte' --count-matches | awk -F':' '{sum += $1} END {print sum}'Length of output: 572
src/lib/ssp/base64.ts (2)
1-7: LGTM: Import and documentation are clear and informative.The import statement is correct, and the JSDoc comment provides a clear explanation of the function's purpose with a helpful reference to MDN for more details.
8-14: LGTM: Function overloads are well-defined and provide flexibility.The two function overloads for
sspBase64are correctly implemented, providing flexibility in usage. The return types are properly specified, including thedefaultValueproperty. This approach aligns with TypeScript best practices for defining multiple function signatures.playground/src/routes/queryparameters/+page.svelte (1)
88-88: LGTM! Input field for base64 parameter added correctly.The input field for the
base64parameter is implemented correctly:
- It uses the appropriate
data-testidattribute for testing.- The two-way data binding with
bind:value={$store.base64}is correct.playground/src/routes/+page.svelte (1)
18-18: LGTM! Consistent with existing pattern.The addition of the
base64_changesvariable is consistent with the existing pattern for tracking changes to other parameters.tests/index.test.ts (1)
109-127: Overall, excellent additions to the test suite!The new test cases for base64 encoding significantly enhance the test coverage for both 'queryParam' and 'queryParameters' functionalities. They are well-structured, cover important scenarios, and maintain consistency across different test suites.
To further improve these additions:
- Consider adding a test for decoding base64 strings to ensure round-trip consistency.
- Add comments explaining the expected behavior in the reactivity test.
- Explore opportunities to reduce duplication by creating shared test utilities.
These changes demonstrate a commitment to thorough testing and will help maintain the reliability of the base64 encoding feature.
Also applies to: 285-293
src/lib/ssp/json-encode.ts (1)
49-49: Export statement is clear and accurateThe export statement correctly exports both
objectEncodeAndDecodeOptionsandarrayEncodeAndDecodeOptionsfunctions, making them available for use in other modules.src/lib/sveltekit-search-params.ts (2)
129-129: Usage ofReturnType<typeof setTimeout>enhances type safetyUpdating
batchTimeoutto useReturnType<typeof setTimeout>improves type safety and ensures compatibility across different environments (Node.js and browsers).
14-14: Verify that './types' correctly exports 'EncodeAndDecodeOptions' and 'StoreOptions'Ensure that the
./typesmodule exportsEncodeAndDecodeOptionsandStoreOptions, and that they are correctly imported here.Run the following script to confirm:
✅ Verification successful
Export Verification Successful
The
./typesmodule correctly exports bothEncodeAndDecodeOptionsandStoreOptions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'EncodeAndDecodeOptions' and 'StoreOptions' are exported from './types.ts' # Test: Search for export statements of the required types. # Expect: Both 'EncodeAndDecodeOptions' and 'StoreOptions' are exported. fd 'types.ts' --type f --exec bash -c 'grep -E "export type (EncodeAndDecodeOptions|StoreOptions)" {}'Length of output: 182
| import ssp from './ssp'; | ||
|
|
||
| export { queryParameters, queryParam } from './sveltekit-search-params'; | ||
| export { ssp }; |
There was a problem hiding this comment.
💡 Codebase verification
Missing ssp.ts File
The ssp.ts file referenced in the imports does not exist in the src/lib/ directory. Please add the ssp.ts module to ensure that imports function correctly.
🔗 Analysis chain
Summary: New ssp module introduced and export structure improved.
The changes in this file introduce a new ssp module and improve the export structure. These modifications align with the PR objective of adding support for base64 string encoding. The code looks good, but it's important to ensure that these changes don't introduce any breaking changes for existing users of the library.
To verify the impact of these changes, please run the following script:
This script will help identify any potential issues with existing imports and provide more context about the new ssp module.
To address the issues encountered with the previous verification scripts, please run the following corrected scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in import statements across the codebase
# Test 1: Check for any remaining imports using the old .js extension
echo "Checking for imports with .js extension:"
rg --type typescript "from ['\"]\.\/sveltekit-search-params\.js['\"]"
# Test 2: Check for any direct imports of ssp that might be affected
echo "Checking for direct imports of ssp:"
rg --type typescript "import .* from ['\"]\.\/ssp['\"]"
# Test 3: Check the contents of the ssp.ts file
echo "Contents of ssp.ts file:"
cat src/lib/ssp.ts
Length of output: 676
Script:
#!/bin/bash
# Description: Corrected scripts to verify import statements and module existence
# Test 1: Check for any remaining imports using the old .js extension
echo "Checking for imports with .js extension:"
rg --type typescript "from ['\"]\.\/sveltekit-search-params\.ts['\"]"
# Test 2: Check for any direct imports of ssp that might be affected
echo "Checking for direct imports of ssp:"
rg --type typescript "import .* from ['\"]\.\/ssp['\"]"
# Test 3: Check the existence of the ssp.ts file
echo "Checking if ssp.ts file exists:"
if [ -f src/lib/ssp.ts ]; then
echo "ssp.ts exists."
else
echo "ssp.ts does not exist."
fi
Length of output: 705
| number: primitiveEncodeAndDecodeOptions({ | ||
| encode: (value: number) => value.toString(), | ||
| decode: (value: string | null) => (value ? parseFloat(value) : null), | ||
| }), | ||
| boolean: primitiveEncodeAndDecodeOptions({ | ||
| encode: (value: boolean) => value + '', | ||
| decode: (value: string | null) => value !== null && value !== 'false', | ||
| }), | ||
| string: primitiveEncodeAndDecodeOptions({ | ||
| encode: (value: string | null) => value ?? '', | ||
| decode: (value: string | null) => value, | ||
| }), |
There was a problem hiding this comment.
Consider refining boolean decoding logic.
The encoding and decoding options for primitive types are generally well-implemented. However, the boolean decoding logic might be too permissive.
Currently, any non-null value that isn't exactly 'false' will be considered true. This could lead to unexpected behavior. Consider using a stricter comparison:
boolean: primitiveEncodeAndDecodeOptions({
encode: (value: boolean) => value + '',
- decode: (value: string | null) => value !== null && value !== 'false',
+ decode: (value: string | null) => value === 'true',
}),This change ensures that only the string 'true' is decoded as true, while everything else (including null) is false.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| number: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: number) => value.toString(), | |
| decode: (value: string | null) => (value ? parseFloat(value) : null), | |
| }), | |
| boolean: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: boolean) => value + '', | |
| decode: (value: string | null) => value !== null && value !== 'false', | |
| }), | |
| string: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: string | null) => value ?? '', | |
| decode: (value: string | null) => value, | |
| }), | |
| number: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: number) => value.toString(), | |
| decode: (value: string | null) => (value ? parseFloat(value) : null), | |
| }), | |
| boolean: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: boolean) => value + '', | |
| decode: (value: string | null) => value === 'true', | |
| }), | |
| string: primitiveEncodeAndDecodeOptions({ | |
| encode: (value: string | null) => value ?? '', | |
| decode: (value: string | null) => value, | |
| }), |
| encode: (value: string) => { | ||
| if (value === '') return undefined; | ||
| const bytes = new TextEncoder().encode(value); | ||
| const binString = Array.from(bytes, (byte) => | ||
| String.fromCodePoint(byte), | ||
| ).join(''); | ||
| return btoa(binString) | ||
| .replace(/\+/g, '-') | ||
| .replace(/\//g, '_') | ||
| .replace(/=/g, ''); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the encode method for readability and performance.
The encode method correctly handles UTF-8 encoding and produces a URL-safe Base64 string. However, it could be optimized for better readability and potentially improved performance.
Consider the following optimizations:
- Use
encodeURIComponentto handle UTF-8 encoding, which is simpler and potentially faster than using TextEncoder. - Use
btoadirectly on the encoded string, eliminating the need for the intermediate binary string conversion. - Use a single
replacecall with a regular expression for URL-safe character replacements.
Here's a suggested implementation:
encode: (value: string) => {
if (value === '') return undefined;
return btoa(encodeURIComponent(value))
.replace(/[+/=]/g, (m) => ({ '+': '-', '/': '_', '=': '' }[m] || m));
},This version is more concise and potentially more efficient, while maintaining the same functionality.
| decode: (value: string | null) => { | ||
| if (value === null) return ''; | ||
| const binString = atob(value.replace(/-/g, '+').replace(/_/g, '/')); | ||
| const bytes = Uint8Array.from(binString, (m) => m.codePointAt(0)!); | ||
| return new TextDecoder().decode(bytes); | ||
| }, | ||
| defaultValue, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the decode method for readability and performance.
The decode method correctly reverses the encoding process, including handling of null input and URL-safe character replacements. However, similar to the encode method, it could be optimized for better readability and potentially improved performance.
Consider the following optimizations:
- Use
decodeURIComponentto handle UTF-8 decoding, which is simpler and potentially faster than using TextDecoder. - Combine the URL-safe character replacements and Base64 decoding into a single step.
Here's a suggested implementation:
decode: (value: string | null) => {
if (value === null) return '';
return decodeURIComponent(
atob(value.replace(/-/g, '+').replace(/_/g, '/'))
.split('')
.map(c => '%' + ('00' + c.charCodeAt(0).toString(16)).slice(-2))
.join('')
);
},This version is more concise and potentially more efficient, while maintaining the same functionality. It also handles potential decoding errors more gracefully by using decodeURIComponent.
| try { | ||
| return JSON.parse( | ||
| decompressFromEncodedURIComponent(value) ?? '', | ||
| ); | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Handle decompression failures before parsing to avoid unnecessary exceptions
If decompressFromEncodedURIComponent(value) returns null or undefined, passing an empty string to JSON.parse`` will cause a SyntaxError. It's better to check for a falsy decompressed value and return null` directly, avoiding unnecessary exceptions.
Apply this diff to improve error handling:
try {
- return JSON.parse(
- decompressFromEncodedURIComponent(value) ?? '',
- );
+ const decompressed = decompressFromEncodedURIComponent(value);
+ if (!decompressed) return null;
+ return JSON.parse(decompressed);
} catch {
return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| return JSON.parse( | |
| decompressFromEncodedURIComponent(value) ?? '', | |
| ); | |
| } catch { | |
| return null; | |
| } | |
| try { | |
| const decompressed = decompressFromEncodedURIComponent(value); | |
| if (!decompressed) return null; | |
| return JSON.parse(decompressed); | |
| } catch { | |
| return null; | |
| } |
| function objectEncodeAndDecodeOptions<T extends object = any>( | ||
| defaultValue: T, | ||
| ): EncodeAndDecodeOptions<T> & { defaultValue: T }; | ||
| function objectEncodeAndDecodeOptions< | ||
| T extends object = any, | ||
| >(): EncodeAndDecodeOptions<T> & { defaultValue: undefined }; | ||
| function objectEncodeAndDecodeOptions<T extends object = any>( | ||
| defaultValue?: T, | ||
| ): EncodeAndDecodeOptions<T> { | ||
| return { | ||
| 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, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication between objectEncodeAndDecodeOptions and arrayEncodeAndDecodeOptions
The functions objectEncodeAndDecodeOptions and arrayEncodeAndDecodeOptions have very similar implementations. This duplication can make maintenance harder and increase the risk of inconsistencies.
Consider creating a generic helper function to handle JSON encoding and decoding for any type, and then use it to implement both functions.
Here's a suggested refactor:
function jsonEncodeAndDecodeOptions<T = any>(
defaultValue?: T,
): EncodeAndDecodeOptions<T> & { defaultValue: T | undefined } {
return {
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 objectEncodeAndDecodeOptions<T extends object = any>(
defaultValue?: T,
): EncodeAndDecodeOptions<T> & { defaultValue: T | undefined } {
return jsonEncodeAndDecodeOptions<T>(defaultValue);
}
function arrayEncodeAndDecodeOptions<T = any>(
defaultValue?: T[],
): EncodeAndDecodeOptions<T[]> & { defaultValue: T[] | undefined } {
return jsonEncodeAndDecodeOptions<T[]>(defaultValue);
}This refactoring reduces code duplication and improves maintainability by centralizing the JSON encoding and decoding logic.
Also applies to: 26-47
| function objectEncodeAndDecodeOptions<T extends object = any>( | ||
| defaultValue: T, | ||
| ): EncodeAndDecodeOptions<T> & { defaultValue: T }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify function overloads for objectEncodeAndDecodeOptions
The overloads for objectEncodeAndDecodeOptions can be combined into a single declaration with an optional parameter, improving readability.
Apply this diff to simplify the overloads:
-function objectEncodeAndDecodeOptions<T extends object = any>(
- defaultValue: T,
-): EncodeAndDecodeOptions<T> & { defaultValue: T };
-function objectEncodeAndDecodeOptions<
- T extends object = any,
->(): EncodeAndDecodeOptions<T> & { defaultValue: undefined };
+function objectEncodeAndDecodeOptions<T extends object = any>(
+ defaultValue?: T,
+): EncodeAndDecodeOptions<T> & { defaultValue: T | undefined };Also applies to: 6-8
| function arrayEncodeAndDecodeOptions<T = any>( | ||
| defaultValue: T[], | ||
| ): EncodeAndDecodeOptions<T[]> & { defaultValue: T[] }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify function overloads for arrayEncodeAndDecodeOptions
Similar to the previous suggestion, the overloads for arrayEncodeAndDecodeOptions can be merged into a single declaration with an optional parameter.
Apply this diff to simplify the overloads:
-function arrayEncodeAndDecodeOptions<T = any>(
- defaultValue: T[],
-): EncodeAndDecodeOptions<T[]> & { defaultValue: T[] };
-function arrayEncodeAndDecodeOptions<T = any>(): EncodeAndDecodeOptions<T[]> & {
- defaultValue: undefined;
-};
+function arrayEncodeAndDecodeOptions<T = any>(
+ defaultValue?: T[],
+): EncodeAndDecodeOptions<T[]> & { defaultValue: T[] | undefined };Also applies to: 29-31
|
This looks good but please rebase it so that i can take a proper look |
This PR is built on top of #116 and should be rebased after that PR is merged or refactor if it is rejected
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests