refactor: refactor encoder #32
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to various files related to testing and configuration. The Changes
Possibly related PRs
Warning Rate limit exceeded@asmyshlyaev177 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (14)
packages/urlstate/testUtils.ts (1)
1-1: Improve type safety and consider implementation limitationsThe implementation is acceptable for basic test scenarios, but there are some improvements to consider:
- Type safety improvements:
-export const clone = (obj: object) => JSON.parse(JSON.stringify(obj)) +export const clone = <T extends Record<string, unknown>>(obj: T): T => + JSON.parse(JSON.stringify(obj))
- Be aware that the current JSON-based implementation has limitations:
- Doesn't preserve
undefinedvalues- Loses function properties
- Doesn't properly clone
Dateobjects (converts to strings)- Throws on circular references
If this utility needs to be used in production code, consider using a more robust solution:
- Use structured cloning:
export const clone = <T>(obj: T): T => structuredClone(obj)
- Or use an established library like lodash:
import { cloneDeep } from 'lodash' export const clone = <T>(obj: T): T => cloneDeep(obj)packages/urlstate/parseSPObj.ts (1)
32-32: Consider adding error loggingThe error handling has been simplified, which is good for maintenance. However, consider adding debug logging to help troubleshoot JSON parsing failures in production.
} catch { + console.debug('Failed to parse JSON:', jsonString); return fallbackValue;packages/urlstate/utils.ts (2)
38-42: Add JSDoc documentation for theisPrimitivefunction.The function implementation is correct, but would benefit from documentation explaining its purpose and return value. Consider adding examples of what constitutes a primitive type.
+/** + * Determines if a value is a primitive type. + * Primitive types include: string, Date, boolean, number, null, undefined, Function, and symbol. + * + * @param {unknown} val - The value to check + * @returns {boolean} True if the value is a primitive type, false otherwise + * + * @example + * isPrimitive("hello") // true + * isPrimitive(new Date()) // true + * isPrimitive({}) // false + * isPrimitive([]) // false + */ export const isPrimitive = (val: unknown): val is Primitive => {
Line range hint
1-54: Consider architectural improvements.
- There are TODO comments indicating missing tests for utility functions. This is particularly important for the new
isPrimitivefunction and type.- The file mixes different concerns (type utilities, router functionality, URL manipulation). Consider splitting these into separate modules for better maintainability:
types.tsfor type definitionsrouter.tsfor router-related codeurl-utils.tsfor URL manipulation utilitiesWould you like me to:
- Generate test cases for the utility functions?
- Propose a file structure reorganization?
🧰 Tools
🪛 Biome
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
tests/useUrlState/main.spec.ts (2)
Line range hint
1-15: Address the TODO comment and consider test organizationThe test suite is well-structured, but there's an unresolved TODO comment regarding a potential error in the
/test-use-clientroute. This should be investigated and documented.Additionally, consider grouping the URL test cases by type (SSR vs Client) for better organization and maintenance.
Would you like me to help investigate the TODO comment or propose a test organization structure?
Line range hint
31-92: Consider standardizing wait times and improving error handlingThe test implementation is solid but could be improved:
- Standardize wait times across tests (currently varying between 200ms, 400ms, and 750ms)
- Add explicit timeouts to
waitForSelectorcalls- Include more descriptive error messages in assertions
Consider refactoring to use constants for wait times:
+ const STANDARD_WAIT = 500; + const SELECTOR_TIMEOUT = { timeout: 5000 }; - await page.waitForTimeout(750); + await page.waitForTimeout(STANDARD_WAIT); - await page.waitForSelector('button[name="Reload page"]'); + await page.waitForSelector('button[name="Reload page"]', SELECTOR_TIMEOUT);packages/urlstate/encodeState/encodeState.test.ts (2)
18-18: LGTM! Consider adding edge cases.The assertion change maintains consistency with the previous test case. The test effectively verifies the round-trip encoding/decoding of nested objects.
Consider adding test cases for:
- Deeply nested objects (3+ levels)
- Arrays containing mixed types
- Objects with special characters in keys
- Empty arrays and objects within nested structures
Example addition:
it('should handle deeply nested structures', () => { const state = { level1: { level2: { level3: { value: 'deep' }, array: [1, 'mixed', { type: true }] } }, 'special@key': 'value', empty: { nested: {} } }; expect(decodeState(encodeState(state))).toStrictEqual(state); });
Line range hint
1-124: Consider enhancing test documentation and performance coverage.The test suite is well-structured with good coverage of functionality. Consider these enhancements:
- Add JSDoc comments to describe the purpose and expectations of each test group
- Add performance tests for large objects:
it('should handle large objects efficiently', () => { const largeState = Array.from({ length: 1000 }, (_, i) => ({ [`key${i}`]: `value${i}`, nested: { data: i } })).reduce((acc, curr) => ({ ...acc, ...curr }), {}); const start = performance.now(); const result = decodeState(encodeState(largeState)); const end = performance.now(); expect(end - start).toBeLessThan(100); // 100ms threshold expect(result).toStrictEqual(largeState); });tests/useUrlState/updateUrl.spec.ts (2)
94-94: Consider making timestamp assertions more flexible.The test uses a hardcoded timestamp which could make it brittle. Consider using regex or timestamp comparison functions to validate the URL structure while being more flexible with the actual datetime values.
Example approach:
const expectedUrlPattern = new RegExp( '\\?name=%27My\\+Name%27&age=55&tags=%5B%7B%27id%27%3A%271%27%2C%27value%27%3A%7B%27text%27%3A%27React\\.js%27%2C%27time%27%3A%27%E2%8F%B2.*Z%27%7D%7D%5D' ); await expect(page.url()).toMatch(expectedUrlPattern);
Line range hint
28-33: Improve test reliability and error handling.Several reliability concerns in the test implementation:
- Using fixed timeouts (
waitForTimeout) and force clicks could make tests flaky- The TODO comment indicates known issues with test reliability
- Error logging is only verified for '/test-ssr-sp' route
Consider:
- Replace
waitForTimeoutwith proper wait conditions (e.g.,waitForResponse,waitForLoadState)- Remove force clicks and fix underlying visibility/interactivity issues
- Implement consistent error checking across all routes
packages/urlstate/utils.test.ts (1)
Line range hint
109-134: Consider improving test readability and assertions.While the test logic is correct, consider these improvements for better maintainability:
- Make test descriptions more specific about the scenarios being tested
- Add explicit assertions for object properties
- Consider adding edge cases (e.g., nested object updates)
Example refactor for the first test case:
- it('should assign a value', () => { + it('should correctly assign a new value while preserving other properties', () => { const result = assignValue(shape, { ...shape, a1: 3 }); const expected = { ...shape, a1: 3 }; expect(clone(result)).toStrictEqual(clone(expected)); + // Add explicit assertions + expect(result.a1).toBe(3); + expect(result.a2).toBe(shape.a2); + expect(result.a3).toEqual(shape.a3); })packages/urlstate/encoder/encoder.ts (1)
104-104: Simplify condition by removing unnecessary optional chaining.The optional chaining
?.instr?.startsWith?.(SYMBOLS.date)is unnecessary becausestris always a string.Apply this diff to simplify the condition:
-if (str?.startsWith?.(SYMBOLS.date)) return new Date(str.slice(1)); +if (str.startsWith(SYMBOLS.date)) return new Date(str.slice(1));packages/urlstate/encoder/encoder.test.ts (2)
139-141: Hardcoded date in test may cause future failuresUsing a fixed future date may lead to unexpected test failures if the system's handling of dates changes over time. Consider using a dynamic date or a date that is relative to the current time.
Optionally, use the current date for the test:
- const d = new Date('2024-06-28T09:10:38.763Z'); + const d = new Date();
230-237: Type casting indecodePrimitivetestsCasting values to
unknown as stringmay hide potential type issues. Ensure that the inputs todecodePrimitiveare correctly typed, or adjust the function to handle these types without casting.Consider refining the tests to avoid unnecessary casting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
jest.config.ts(1 hunks)package.json(2 hunks)packages/urlstate/encodeState/encodeState.test.ts(2 hunks)packages/urlstate/encoder/encoder.test.ts(4 hunks)packages/urlstate/encoder/encoder.ts(3 hunks)packages/urlstate/parseSPObj.ts(2 hunks)packages/urlstate/testUtils.ts(1 hunks)packages/urlstate/utils.test.ts(1 hunks)packages/urlstate/utils.ts(1 hunks)test.env.ts(0 hunks)tests/useUrlState/main.spec.ts(1 hunks)tests/useUrlState/updateUrl.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- test.env.ts
🧰 Additional context used
🪛 Biome
packages/urlstate/encoder/encoder.test.ts
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 268-269: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/urlstate/encoder/encoder.ts
[error] 54-54: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 55-55: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 56-56: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/urlstate/utils.ts
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (20)
jest.config.ts (1)
7-8: Remove commented code and verify JSDOM compatibility.
The switch to the standard JSDOM environment is a good practice, but let's ensure everything works as expected:
- Remove the commented line as it's no longer needed and adds noise
- Verify that the standard JSDOM environment provides all required functionality (e.g., structuredClone)
testEnvironment: "jsdom",
- // testEnvironment: "./test.env.ts",✅ Verification successful
Remove commented code - environment change is safe
The switch to JSDOM is safe to proceed with:
- Jest 29.7.0 and jest-environment-jsdom 29.7.0 are modern versions that include full support for structuredClone
- No custom environment file exists in the codebase anymore
- Test files only show standard Jest mocking and setup, with no dependency on custom environment features
testEnvironment: "jsdom",
- // testEnvironment: "./test.env.ts",🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test compatibility with JSDOM environment
# Check if tests use structuredClone or other custom environment features
# Search for structuredClone usage in test files
echo "Checking for structuredClone usage in tests:"
rg "structuredClone" -g "*.test.*"
# Look for potential custom environment dependencies
echo -e "\nChecking for other potential custom environment dependencies:"
rg "global\." -g "*.test.*"
Length of output: 302
Script:
#!/bin/bash
# Let's check if the custom test environment file exists and its content
echo "Checking for test.env.ts:"
fd "test.env.ts"
# Let's also check test files for any specific environment setup or mocks
echo -e "\nChecking test files for environment setup:"
rg "beforeAll|beforeEach|jest\.mock" -g "*.test.*" -A 2
# Check package.json for jest version to confirm JSDOM compatibility
echo -e "\nChecking package.json for jest version:"
cat package.json | grep -A 2 '"jest":'
Length of output: 2407
packages/urlstate/parseSPObj.ts (2)
1-2: LGTM: Clean import refactoring
The imports have been simplified and properly aligned with the new type system using CustomDecoded.
28-28: Document the return type change and verify its impact
The return type change from T | Primitive | undefined to T | CustomDecoded | undefined could be a breaking change for consumers of this API.
Let's check for usages of this function:
Consider:
- Updating the function's JSDoc to document the new return type
- Adding a migration guide if this is a breaking change
tests/useUrlState/main.spec.ts (1)
19-20: Verify the Unicode encoding change in the URL
The URL now includes a Unicode character encoding (%E2%8F%B2) before the timestamp. While this works, it's important to ensure:
- This encoding is intentional and documented
- The decoder properly handles this across different browsers
- The encoding is consistent with the project's URL encoding strategy
Let's verify the encoding implementation:
packages/urlstate/encodeState/encodeState.test.ts (1)
8-8: Improved test assertion structure!
The reordered assertion expect(decodeState(encodeState(state))).toStrictEqual(state) better follows testing best practices by making the actual value (decoded state) the subject of the assertion.
Let's check if this pattern is consistently applied across other test files:
✅ Verification successful
Test assertion pattern is consistently applied
The search results confirm that the improved assertion pattern expect(decodeState(encodeState(state))).toStrictEqual(state) is consistently used throughout the test file, appearing in multiple test cases. This consistent pattern makes the tests more readable and maintainable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar assertion patterns in other test files
rg -l "expect\(.*state.*\)\.toStrictEqual\(decodeState\(encodeState\(" || rg -l "expect\(state\)\.toStrictEqual\(decodeState\("
Length of output: 128
Script:
#!/bin/bash
# Let's try a broader search for test assertions involving encodeState and decodeState
rg "expect.*decodeState.*encodeState" -A 2 --type ts
Length of output: 812
tests/useUrlState/updateUrl.spec.ts (1)
151-151: Verify timestamp encoding convention.
The URL shows inconsistent timestamp encoding - 'time' field includes the clock emoji prefix (%E2%8F%B2) while 'iso' field doesn't. Please verify if this is intentional or if all date fields should follow the same encoding convention.
✅ Verification successful
Let me gather more specific information about the timestamp handling.
Let me check the encoder implementation to understand how dates are handled.
Based on the test files and constants, I can now provide the final response:
The timestamp encoding is correctly implemented
The clock emoji (⏲) prefix is intentionally used only for Date objects/instances, while ISO string timestamps remain unprefixed. This is confirmed by:
- The constants file defines the date prefix as "⏲"
- The encoder tests explicitly verify this behavior:
- Date instances are encoded with "⏲" prefix:
encodePrimitive(d)).toStrictEqual('⏲2024-06-28T09:10:38.763Z') - ISO string dates remain unprefixed:
encodePrimitive(d)).toStrictEqual("2024-06-28T09:10:38.763Z")
- Date instances are encoded with "⏲" prefix:
In the test URL, 'time' field uses the clock emoji because it's encoded as a Date object, while 'iso' field is stored as a string, making this a correct implementation of the encoding convention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other timestamp encodings in the codebase to verify the convention
rg -l "time.*%E2%8F%B2.*Z" --type ts
rg -l "iso.*Z" --type ts
# Check the encoder implementation
ast-grep --pattern 'function $_(date: Date) {
$$$
}'
Length of output: 382
Script:
#!/bin/bash
# Check the actual test cases and implementation details
rg -A 5 -B 5 "time.*%E2%8F%B2.*Z" tests/useUrlState/updateUrl.spec.ts
rg -A 5 -B 5 "time.*%E2%8F%B2.*Z" tests/useUrlState/main.spec.ts
# Look for any date formatting or encoding functions
ast-grep --pattern 'function $_(date$_) {
$$$
}'
# Search for clock emoji usage in the codebase
rg "⏲" --type ts
Length of output: 1869
Script:
#!/bin/bash
# Check the encoder implementation for date handling
rg -A 10 "date:" packages/urlstate/constants/constants.ts
cat packages/urlstate/encoder/encoder.test.ts
# Look for the actual encoding/decoding implementation
ast-grep --pattern 'function encodePrimitive($_) {
$$$
}'
ast-grep --pattern 'function decodePrimitive($_) {
$$$
}'
Length of output: 8500
packages/urlstate/utils.test.ts (1)
3-4: LGTM! Good modularization.
Moving the clone function to a separate utility file improves code organization and reusability.
package.json (2)
345-345: LGTM: Safe patch update for @types/jest
The update from ^29.5.13 to ^29.5.14 is a patch version increment that should only contain bug fixes.
377-377: Verify ts-jest minor version update compatibility
The update from ^29.1.5 to ^29.2.5 is a minor version increment that may introduce new features or behavior changes.
Let's verify the changes and any potential impacts:
✅ Verification successful
ts-jest update from ^29.1.5 to ^29.2.5 appears safe to proceed
The update is safe based on:
- Basic preset configuration in jest.config.ts without custom transformers
- No complex ts-jest configurations that might be affected by version changes
- Package.json and package-lock.json show clean version resolution
- The minor version bump (29.1.x to 29.2.x) follows semver, indicating backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ts-jest changelog and run tests to verify compatibility
# Get the changelog between versions
echo "Checking ts-jest changelog between v29.1.5 and v29.2.5..."
gh api repos/kulshekhar/ts-jest/releases \
--jq '.[] | select(.tag_name | startswith("v29.")) | {tag: .tag_name, body: .body}' \
| jq 'select(.tag | startswith("v29.2") or .tag == "v29.1.5")'
# Verify if tests pass with the new version
echo "Checking if tests pass..."
npm run test:unit
Length of output: 2221
Script:
#!/bin/bash
# Let's check the ts-jest configuration and usage in the codebase
# Find ts-jest configuration files
echo "Checking for ts-jest configuration files..."
fd -e js -e json -e ts "jest.config"
# Find test files using ts-jest
echo "Looking for test files using ts-jest..."
rg -l "ts-jest" --type ts --type js --type json
# Check package-lock.json for ts-jest version history
echo "Checking package-lock.json for ts-jest version details..."
rg "ts-jest" package-lock.json -A 5 -B 5
Length of output: 2145
Script:
#!/bin/bash
# Let's examine the jest configuration to understand ts-jest usage
# Check jest configuration
echo "Examining jest.config.ts content..."
cat jest.config.ts
# Look for any specific ts-jest configurations or presets
echo "Looking for ts-jest specific configurations..."
rg "transform|preset" jest.config.ts -A 2 -B 2
Length of output: 794
packages/urlstate/encoder/encoder.ts (3)
17-20: Encoding function updated correctly with replacer.
The encode function now uses the replacer in JSON.stringify to handle custom serialization, which enhances the encoding process.
22-43: Recursive replacer function handles complex data structures effectively.
The replacer function correctly processes objects and arrays by recursively encoding their values. This ensures that nested structures are appropriately serialized.
75-75: Potential type assertion issue in decode function.
The function decode<T> casts the result of parseJSON to type T, but parseJSON may return CustomDecoded or undefined, which might not align with T. This could lead to runtime type errors.
Consider refining the return type or adding type guards to ensure safety.
packages/urlstate/encoder/encoder.test.ts (8)
4-4: Importing encodePrimitive function is appropriate
Adding encodePrimitive to the imports ensures that the function is available for testing. This aligns with the updates in the test cases.
10-10: Importing clone utility function
Including clone from ../testUtils aids in creating deep copies of objects for testing purposes, preventing unintended side effects.
14-28: Comprehensive tests for simple object encoding and decoding
The tests cover encoding and decoding of a simple object with various data types, including handling of undefined properties.
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
31-53: Well-structured tests for nested object encoding and decoding
The nested object tests effectively verify the encoder's ability to handle complex structures with nested objects and arrays.
Line range hint 55-84: Thorough tests for all primitive types in nested objects
The tests ensure that all primitive types are correctly encoded and decoded within nested objects, enhancing test coverage.
112-170: Good addition of comprehensive tests for encodePrimitive function
The added tests cover various primitive types, including strings with special characters, numbers, dates (both instances and ISO strings), booleans, null, undefined, functions, and symbols. This ensures that encodePrimitive handles all expected cases correctly.
164-170: Handling of unsupported types in encodePrimitive
The test correctly verifies that functions and symbols are encoded to empty strings. This is acceptable since these types cannot be serialized in JSON.
230-237: Ensure decodePrimitive handles arrays and objects appropriately
decodePrimitive is being tested with arrays and objects cast to strings, which might not be the intended use. Verify that decodePrimitive should accept only primitive types.
Consider whether decodePrimitive should handle arrays and objects, or if these cases should be handled elsewhere.
asmyshlyaev177
force-pushed
the
encoder_refactor
branch
from
91be8c1 to
9320e19
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
packages/urlstate/encoder/encoder.ts (2)
37-39: Consider optimizing array handling performanceThe current implementation creates a new array via map for every nested array. Consider using a for-loop for better performance with large arrays, as it avoids creating intermediate arrays.
- return (value as unknown as Array<unknown>).map( - (val) => replacer("", val) as Simple, - ); + const arr = value as unknown as Array<unknown>; + const result = new Array(arr.length); + for (let i = 0; i < arr.length; i++) { + result[i] = replacer("", arr[i]) as Simple; + } + return result;
88-92: Enhance error handling in parseJSONThe current implementation silently falls back to the fallback value for any parsing error. Consider logging parsing errors in development and distinguishing between different error types.
try { return JSON.parse(jsonString, reviver) as T; } catch (error) { + if (process.env.NODE_ENV !== 'production') { + console.warn('Failed to parse JSON:', error); + } return fallbackValue; }packages/urlstate/utils.ts (1)
38-42: Consider renamingisPrimitiveto better reflect its purpose.The function name
isPrimitiveis misleading since it includes non-primitive types likeDateandFunction. Consider renaming it toisSimpleValueorisNonCompositeValueto better reflect its actual behavior.-export const isPrimitive = (val: unknown): val is Simple => { +export const isSimpleValue = (val: unknown): val is Simple => { const type = typeOf(val); return type !== "object" && type !== "array"; };packages/urlstate/utils.test.ts (2)
Line range hint
82-116: Consider adding a test for nested object mutations.The test suite thoroughly covers basic scenarios, but it would be beneficial to add a test case that verifies nested objects are properly cloned and not mutated. This ensures deep immutability.
Here's a suggested test case:
it('should not mutate nested objects', () => { const original = clone(shape); const result = assignValue(shape, { a3: { a4: 'modified' } }); // Verify original wasn't mutated expect(shape).toStrictEqual(original); // Verify nested change was applied expect(result.a3.a4).toBe('modified'); // Verify it's a new object instance expect(result.a3).not.toBe(shape.a3); });
196-237: Consider adding type safety tests.The test suite is comprehensive for runtime behavior, but since this is a TypeScript project, consider adding type safety tests to ensure the function maintains type constraints.
Here's a suggested test case that verifies type safety:
it('maintains type safety with different value types', () => { interface TestShape { num: number; str: string; bool: boolean; } const shape: TestShape = { num: 0, str: '', bool: false }; const entries: [string, string][] = [ ['num', '42'], ['str', 'hello'], ['bool', 'true'], ['extra', 'value'] ]; const result = filterUnknown(shape, entries); // TypeScript should enforce this type const typed: [keyof TestShape, string][] = result; expect(typed).toBeDefined(); });packages/urlstate/encoder/encoder.test.ts (1)
Line range hint
214-215: Consider using proper benchmarking toolsInstead of using
console.logfor performance measurements, consider using dedicated benchmarking tools or test frameworks that support performance testing (e.g., Jest's built-in timer mocks or libraries likebenchmark.js).🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
jest.config.ts(1 hunks)package.json(2 hunks)packages/urlstate/encodeState/encodeState.test.ts(2 hunks)packages/urlstate/encoder/encoder.test.ts(4 hunks)packages/urlstate/encoder/encoder.ts(3 hunks)packages/urlstate/parseSPObj.ts(2 hunks)packages/urlstate/testUtils.ts(1 hunks)packages/urlstate/utils.test.ts(2 hunks)packages/urlstate/utils.ts(2 hunks)test.env.ts(0 hunks)tests/useUrlState/main.spec.ts(1 hunks)tests/useUrlState/updateUrl.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- test.env.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- jest.config.ts
- package.json
- packages/urlstate/encodeState/encodeState.test.ts
- packages/urlstate/parseSPObj.ts
- packages/urlstate/testUtils.ts
- tests/useUrlState/main.spec.ts
- tests/useUrlState/updateUrl.spec.ts
🧰 Additional context used
🪛 Biome
packages/urlstate/encoder/encoder.test.ts
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 268-269: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/urlstate/utils.ts
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (7)
packages/urlstate/encoder/encoder.ts (1)
72-72: Consider adding runtime type validation
The current type casting to T could lead to runtime errors if the decoded value doesn't match the expected type. Consider adding runtime type validation or using a type validation library like io-ts or zod.
packages/urlstate/utils.ts (1)
44-53: Past review comment is still applicable.
The previous review comment about refining the Simple type definition remains valid and addresses the current implementation's issues.
🧰 Tools
🪛 Biome
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
packages/urlstate/utils.test.ts (1)
1-3: LGTM! Import changes align with the refactoring.
The imports reflect the architectural improvement of moving the clone utility to a separate file and making filterUnknown a public function.
packages/urlstate/encoder/encoder.test.ts (4)
24-26: Replace delete operator with undefined assignment
For better performance, avoid using the delete operator as it can cause the JavaScript engine to deoptimize the code.
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
112-170: Well-structured and comprehensive test coverage for encodePrimitive!
The test suite thoroughly covers all primitive types, edge cases, and special characters. Good job on organizing the tests with clear descriptions and meaningful assertions.
267-269: Replace delete operator with undefined assignment
For better performance, avoid using the delete operator.
🧰 Tools
🪛 Biome
[error] 268-269: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
230-237: Good coverage of type handling in decodePrimitive!
The test cases thoroughly verify the handling of various types including null, boolean, numeric, array, and object values. The type assertions are well-structured and comprehensive.
asmyshlyaev177
force-pushed
the
encoder_refactor
branch
from
9e2f496 to
d9e905d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (11)
packages/urlstate/encoder/encoder.ts (4)
17-19: Consider swapping the order of string replacementsThe current order of replacements could lead to double encoding in some edge cases. Consider swapping the order:
- return JSON.stringify(payload, replacer) - .replace(/'/g, "%27") - .replace(/"/g, "'"); + return JSON.stringify(payload, replacer) + .replace(/"/g, "'") + .replace(/'/g, "%27");
22-43: Optimize replacer function implementationThe function implementation could be optimized in several ways:
function replacer(_key: string, value: unknown): unknown { const type = typeOf(value); + if (value === null) return value; if (type !== "object" && type !== "array") { return encodePrimitive(value as unknown as Simple); } if (type === "object") { - const _value = value as { [key: string]: unknown }; + const _value = value as Record<string, unknown>; Object.keys(_value).forEach((objKey) => { _value[objKey] = replacer("", _value[objKey]); }); return _value; } if (type === "array") { - return (value as unknown as Array<unknown>).map( - (val) => replacer("", val) as Simple, - ); + return Array.isArray(value) + ? value.map(val => replacer("", val)) + : value; } return value; }
84-92: Enhance error handling in parseJSONConsider adding more detailed error handling to help with debugging:
export function parseJSON<T extends JSONCompatible>( jsonString: string, fallbackValue?: T, ): T | CustomDecoded | undefined { try { return JSON.parse(jsonString, reviver) as T; } catch (error) { + if (process.env.NODE_ENV !== 'production') { + console.warn( + `Failed to parse JSON: ${error instanceof Error ? error.message : 'Unknown error'}`, + '\nInput:', jsonString + ); + } return fallbackValue; } }
95-97: Add null check to reviver functionThe current implementation might not handle null values correctly:
export function reviver(_key: string, value: unknown): unknown { - return typeof value === "string" ? decodePrimitive(value) : value; + return value !== null && typeof value === "string" ? decodePrimitive(value) : value; }packages/urlstate/utils.ts (1)
103-108: Add JSDoc documentation for the exported function.Since this function is now exported (public API), it should have proper documentation explaining its purpose, parameters, and return value.
Add JSDoc documentation:
+/** + * Filters an array of key-value pairs based on keys present in the shape object + * @param shape - Object defining the allowed keys + * @param entries - Array of key-value pairs to filter + * @returns Filtered array of key-value pairs with spaces decoded + */ export function filterUnknown<T extends object>( shape: T, entries: [key: string, value: string][], ) {packages/urlstate/utils.test.ts (2)
Line range hint
92-127: Consider improving test readability.While the test cases are comprehensive, their readability could be enhanced by:
- Using more descriptive test names that explain the business logic
- Adding comments to explain the purpose of complex assertions
Example refactor:
- it('should assign a value', () => { + it('should correctly assign a new value while preserving other properties', () => { expect(clone(assignValue(shape, { ...shape, a1: 3 }))).toStrictEqual(clone({ ...shape, a1: 3, })) }) - it('should return a new instance of object', () => { + it('should return a new object instance to maintain immutability', () => { const newVal = {} const result = assignValue(shape, newVal) + // Verify that neither the input objects are returned directly expect(result === shape).toBeFalsy() expect(result === newVal).toBeFalsy() })
196-237: Consider enhancing type safety and test coverage.The test suite is well-structured, but could be improved in two areas:
- Type Safety: The entries type could be more precise using a type alias
- Test Coverage: Add tests for more complex scenarios
Example improvements:
// Define a type alias for better type safety type Entry = [string, string]; // Add more complex test scenarios it('handles nested shape objects correctly', () => { const shape = { a: { nested: '' }, b: '' }; const entries: Entry[] = [ ['a.nested', 'value1'], ['b', 'value2'], ['c', 'value3'], ]; const result = filterUnknown(shape, entries); expect(result).toEqual([ ['a.nested', 'value1'], ['b', 'value2'], ]); }); it('handles array values in shape', () => { const shape = { arr: [''], b: '' }; const entries: Entry[] = [ ['arr.0', 'value1'], ['b', 'value2'], ]; const result = filterUnknown(shape, entries); expect(result).toEqual([ ['arr.0', 'value1'], ['b', 'value2'], ]); });packages/urlstate/encoder/encoder.test.ts (4)
89-109: Consider adding edge cases to array testsThe array tests could be more comprehensive by including:
- Empty arrays
- Arrays with undefined/null elements
- Arrays with mixed types
170-228: Consider adding error handling tests for encodePrimitiveThe test suite should include cases for invalid inputs to ensure proper error handling:
- Non-serializable objects
- Circular references
- Invalid date strings
Line range hint
271-275: Remove console.log and consider using jest.spyOn for performance testsPerformance measurements in tests can be unreliable due to varying system conditions. Consider:
- Removing the console.log statement
- Using jest.spyOn for performance critical operations if timing is crucial
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 136-136: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
289-294: Consider restructuring type assertions in testsThe current approach of using type assertions (
as unknown as string) might hide potential runtime issues. Consider creating separate test cases for different input types or using proper type guards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
jest.config.ts(1 hunks)package.json(2 hunks)packages/urlstate/encodeState/encodeState.test.ts(2 hunks)packages/urlstate/encoder/encoder.test.ts(3 hunks)packages/urlstate/encoder/encoder.ts(3 hunks)packages/urlstate/parseSPObj.ts(2 hunks)packages/urlstate/testUtils.ts(1 hunks)packages/urlstate/utils.test.ts(2 hunks)packages/urlstate/utils.ts(2 hunks)test.env.ts(0 hunks)tests/useUrlState/main.spec.ts(1 hunks)tests/useUrlState/updateUrl.spec.ts(2 hunks)
💤 Files with no reviewable changes (1)
- test.env.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- jest.config.ts
- package.json
- packages/urlstate/encodeState/encodeState.test.ts
- packages/urlstate/parseSPObj.ts
- packages/urlstate/testUtils.ts
- tests/useUrlState/main.spec.ts
- tests/useUrlState/updateUrl.spec.ts
🧰 Additional context used
🪛 Biome
packages/urlstate/encoder/encoder.test.ts
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 136-136: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 326-327: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/urlstate/utils.ts
[error] 52-52: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (5)
packages/urlstate/encoder/encoder.ts (1)
109-109: LGTM! Good use of ReturnType utility type
The CustomDecoded type definition is clean and maintainable.
packages/urlstate/utils.ts (1)
109-117:
Improve robustness of entry processing.
The current implementation has two issues:
- Space replacement (
+to space) is only applied to keys, not values - Array access is not properly guarded against malformed entries
Apply this safer implementation:
- const result = [];
+ const result: [string, string][] = [];
for (let i = 0; i < entries.length; i++) {
- if (shapeKeys.includes(entries[i]?.[0])) {
- result.push([entries[i][0].replace(/\+/g, " "), entries[i][1]]);
+ const entry = entries[i];
+ if (!Array.isArray(entry) || entry.length !== 2) {
+ continue;
+ }
+ const [key, value] = entry;
+ if (typeof key === 'string' && typeof value === 'string' &&
+ shapeKeys.includes(key)) {
+ result.push([
+ key.replace(/\+/g, " "),
+ value.replace(/\+/g, " ")
+ ]);
}
}Likely invalid or redundant comment.
packages/urlstate/utils.test.ts (1)
1-3: LGTM! Good separation of concerns.
The imports have been properly organized, with test utilities moved to a dedicated file.
packages/urlstate/encoder/encoder.test.ts (2)
25-26: Replace delete operator with undefined assignment
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
326-327: Replace delete operator with undefined assignment
🧰 Tools
🪛 Biome
[error] 326-327: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/urlstate/encoder/encoder.test.ts (4)
89-109: Consider adding more edge cases for array testingWhile the current tests cover basic scenarios well, consider adding tests for:
- Empty arrays
- Arrays with undefined/null elements
- Very large arrays (performance edge case)
188-193: Add boundary value tests for numbersConsider adding tests for number edge cases:
- Maximum/minimum safe integers
- NaN
- Infinity/-Infinity
Line range hint
271-274: Replace console.log with proper performance testingInstead of using
console.logfor performance metrics, consider:
- Using Jest's built-in timer mocks
- Implementing proper performance benchmarks using tools like
benchmark.js- Moving performance tests to a separate performance testing suite
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
288-295: Improve type handling in test casesThe current approach of type casting (
as unknown as string) is verbose and potentially fragile. Consider:
- Creating proper test fixtures with correct types
- Using TypeScript's type assertions more effectively
- Creating helper functions for type conversion if needed
Example:
// Instead of: expect(decodePrimitive(false as unknown as string)) // Consider: function createTestCase<T>(value: T): string { return String(value); } expect(decodePrimitive(createTestCase(false)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/urlstate/encoder/encoder.test.ts(3 hunks)
🧰 Additional context used
🪛 Biome
packages/urlstate/encoder/encoder.test.ts
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 326-327: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
packages/urlstate/encoder/encoder.test.ts (2)
4-4: LGTM! Good test isolation practice
The addition of clone utility helps maintain test isolation by preventing mutation of test data between test cases.
Also applies to: 10-10
13-87: Well-structured and comprehensive test coverage
The test suite effectively covers:
- Simple object encoding/decoding
- Nested object structures
- All primitive types
- Edge cases
🧰 Tools
🪛 Biome
[error] 25-26: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Please follow contributing guidelines
PR title
Proposed Changes
Breaking Changes