use first argument for defaultState
#24
Conversation
WalkthroughThe changes in this pull request primarily focus on updating the 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 (
|
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: 1
🧹 Outside diff range and nitpick comments (13)
packages/example-nextjs14/src/app/Status-for-test.tsx (1)
7-7: LGTM! Consider adding JSDoc for better documentation.The new hook usage pattern is cleaner and more intuitive, with
formas the first argument and options as the second. The variable nameurlStatebetter describes its purpose.Consider adding JSDoc to document the hook's usage:
+ /** + * Hook to sync form state with URL + * @param {typeof form} defaultState - Initial form state + * @param {object} options - Configuration options including searchParams + */ const { urlState } = useUrlState(form, { searchParams: sp });packages/example-nextjs14/src/app/components/CodeBlocksRR.tsx (1)
22-23: Consider documenting thereplaceoption's purpose.While the documentation link is helpful, it would be valuable to add a brief comment explaining what the
replace: trueoption does, as this is a key difference from the other example.// see docs for all possible params https://github.com/asmyshlyaev177/state-in-url/tree/master/packages/urlstate/react-router/useUrlState +// Using replace: true will replace the URL history entry instead of adding a new one const { urlState, setUrl, setState } = useUrlState(form, { replace: true });packages/example-nextjs14/src/app/components/CodeBlocksNext.tsx (1)
26-27: Documentation enhancement suggestion.The added comments about
useHistoryand documentation link are helpful. Consider adding a brief example of the options object structure to make it even clearer.// see docs for all possible params https://github.com/asmyshlyaev177/state-in-url/tree/master/packages/urlstate/next/useUrlState + // Example options: { useHistory: true, searchParams, key: 'myState' }packages/example-nextjs15/src/app/Form-for-test.tsx (1)
18-18: Consider simplifying thereplacecondition.The ternary expression can be simplified to a direct boolean comparison.
- replace: sp.get('replace') === 'false' ? false : true, + replace: sp.get('replace') !== 'false',🧰 Tools
🪛 Biome
[error] 18-18: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/example-nextjs14/src/app/Form-for-test.tsx (1)
17-20: Simplify the boolean expression in the options object.The ternary operator for the
replaceoption can be simplified.const { urlState, setState, setUrl } = useUrlState(form, { searchParams, - replace: sp.get('replace') === 'false' ? false : true, + replace: sp.get('replace') !== 'false', });🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/example-react-router6/src/Form-for-test.tsx (1)
9-11: Consider improving the replace option logic.The current implementation could be more robust and clearer:
- replace: sp.get("replace") === "false" ? false : true, + replace: sp.get("replace")?.toLowerCase() !== "false",This change:
- Handles case-insensitive comparison
- Is more concise
- Maintains the same behavior
🧰 Tools
🪛 Biome
[error] 10-10: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
README.md (1)
429-429: LGTM! Consider enhancing custom hook documentation.Both examples correctly demonstrate the new API. For the custom hook implementation, consider adding a comment or example showing how to pass options through the custom hook if needed.
Add a comment above the custom hook:
/** * Custom hook for managing user state in URL * @example * // Basic usage * const { userState, setUserState } = useUserState(); * * // With options * const { userState, setUserState } = useUserState({ searchParams }); */Also applies to: 543-543
packages/urlstate/next/useUrlState/useUrlState.ts (4)
15-19: Clarify the deprecation message for better guidanceThe deprecation comment lacks specific information. Providing a clear message helps users understand the change and the recommended alternative.
Consider updating the deprecation comment:
/** - * @deprecated . + * @deprecated Use `useUrlState(defaultState, { ...otherParams })` instead. * */
69-92: Leverage TypeScript overloads for cleaner parameter handlingCurrently, the function differentiates parameter formats at runtime by checking if
defaultStatehas adefaultStateproperty. Using TypeScript function overloads can improve type safety and code clarity by handling different parameter signatures at compile time.Consider refactoring with explicit overloads:
export function useUrlState<T extends JSONCompatible>( params: OldParams<T> ): ReturnType; export function useUrlState<T extends JSONCompatible>( defaultState: T, params?: Params ): ReturnType; export function useUrlState<T extends JSONCompatible>( defaultStateOrParams: T | OldParams<T>, params?: Params ) { // Implementation... }
80-84: Set default value for_useHistoryto ensure consistent behaviorIf both
defaultState.useHistoryandparams?.useHistoryare undefined,_useHistorybecomesundefined, which may lead to unexpected behavior. It's advisable to provide a default value to guarantee_useHistoryis always a boolean.Modify the assignment to include a default:
const _useHistory = ( "defaultState" in defaultState ? defaultState.useHistory : params?.useHistory -) as boolean; +) ?? true;
155-159: Provide a clear deprecation message forupdateUrlThe deprecation comment for
updateUrlcan be improved to guide users toward the preferredsetUrlmethod.Update the deprecation notice:
/** - * @deprecated use `setUrl` + * @deprecated Use `setUrl` instead. */packages/urlstate/react-router/useUrlState/useUrlState.ts (2)
18-22: Enhance deprecation message for clarityThe deprecation notice could be more informative. Please specify when this function will be removed and guide users on the preferred alternative.
70-88: Simplify parameter handling for better readabilityThe current parameter handling logic uses type checking with
"defaultState" in defaultState, which can be confusing and affects readability. Consider using function overloading or type guards to distinguish between different parameter types more clearly.
📜 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 (17)
README.md(5 hunks)packages/example-nextjs14/src/app/(tests)/useUrlState/Comp1.tsx(2 hunks)packages/example-nextjs14/src/app/Form-for-test.tsx(1 hunks)packages/example-nextjs14/src/app/Form.tsx(1 hunks)packages/example-nextjs14/src/app/Status-for-test.tsx(2 hunks)packages/example-nextjs14/src/app/Status.tsx(2 hunks)packages/example-nextjs14/src/app/components/CodeBlockState.tsx(1 hunks)packages/example-nextjs14/src/app/components/CodeBlocksNext.tsx(4 hunks)packages/example-nextjs14/src/app/components/CodeBlocksRR.tsx(4 hunks)packages/example-nextjs15/src/app/(tests)/useUrlState/Comp1.tsx(2 hunks)packages/example-nextjs15/src/app/Form-for-test.tsx(1 hunks)packages/example-nextjs15/src/app/Status-for-test.tsx(2 hunks)packages/example-react-router6/src/FewComponents/Component.tsx(1 hunks)packages/example-react-router6/src/Form-for-test.tsx(1 hunks)packages/example-react-router6/src/Status-for-test.tsx(2 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(4 hunks)packages/urlstate/react-router/useUrlState/useUrlState.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/example-nextjs14/src/app/components/CodeBlockState.tsx
🧰 Additional context used
🪛 Biome
packages/example-nextjs14/src/app/Form-for-test.tsx
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (31)
packages/example-react-router6/src/FewComponents/Component.tsx (1)
8-10: LGTM! Verify consistent usage across codebase.
The updated useUrlState hook usage correctly implements the new signature, passing the state shape directly as the first argument and options as the second argument.
Let's verify that this new pattern is consistently applied across the codebase:
✅ Verification successful
All usages of useUrlState follow the new signature pattern
Based on the comprehensive search results, all instances of useUrlState across the codebase consistently follow the new signature pattern where:
- The first argument is the state object/shape directly
- The second argument is an optional configuration object (when needed) containing options like
replace,searchParams, etc.
The changes in packages/example-react-router6/src/FewComponents/Component.tsx align with this pattern and match how the hook is used throughout the codebase, including examples in Next.js and React Router implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useUrlState across the codebase
# Expect: All useUrlState calls should follow the new pattern (direct state object, optional config object)
# Search for all useUrlState usage
rg -A 2 "useUrlState\(" --type tsx --type ts
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach without file type filtering
# Search for useUrlState usage patterns across all files
rg "useUrlState\(" -A 2
# Also search for the hook definition to understand its signature
rg "function useUrlState"
Length of output: 9131
packages/example-nextjs14/src/app/(tests)/useUrlState/Comp1.tsx (1)
17-19: LGTM! Clean implementation of state updates.
The select component correctly implements the new API, using urlState for reading and setUrl with an immutable update pattern for writing. The implementation is clean and type-safe.
packages/example-nextjs15/src/app/(tests)/useUrlState/Comp1.tsx (3)
8-11: LGTM! Hook usage aligns with the new API design.
The updated hook usage correctly implements the new parameter structure, where stateShape is the first argument and options are passed as the second argument. This matches the PR's objective of streamlining the API.
17-20: LGTM! State updates are handled correctly.
The implementation properly:
- Uses the new
urlStateaccessor for reading state - Maintains immutability by spreading the current state in the update
- Correctly coerces the value to a number using the
+operator
5-5: Verify stateShape implementation and usage consistency.
Let's ensure the stateShape and hook usage pattern is consistent across the codebase.
✅ Verification successful
Let me gather more information about the usage pattern since the first attempt didn't capture all variations.
Let me check the implementation of the components that use stateShape to verify consistency.
Let me fix the regex and check the implementations.
Based on the verification results, I can now provide the final response:
Implementation of stateShape and its usage is consistent ✓
The stateShape import and implementation is correctly used across the codebase:
- Defined consistently in state.ts files with the same structure
- Used properly with useUrlState hook in components
- Follows the same pattern in both Next.js and React Router examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify stateShape implementation and hook usage pattern
# Check stateShape implementation
echo "Checking stateShape implementation:"
rg -A 5 "export const stateShape"
# Check hook usage pattern across components
echo "\nChecking useUrlState hook usage pattern:"
ast-grep --pattern 'const { $urlState, $setUrl } = useUrlState($stateShape, $options)'
Length of output: 1259
Script:
#!/bin/bash
# Let's check for different patterns of useUrlState usage
echo "Checking useUrlState imports and usage:"
rg -A 3 "useUrlState"
# Also check for files importing stateShape
echo "\nChecking stateShape imports:"
rg "import.*stateShape"
Length of output: 46537
Script:
#!/bin/bash
# Check implementation of components using stateShape
echo "Checking components using stateShape:"
rg -A 10 "stateShape.*=.*{" --type ts --type tsx
# Check for any other components in the same directory
echo "\nChecking other components in the directory:"
fd -e tsx -e ts . "packages/example-nextjs15/src/app/(tests)/useUrlState/"
Length of output: 800
Script:
#!/bin/bash
# Check stateShape implementation
echo "Checking stateShape implementation in state.ts:"
cat packages/example-nextjs15/src/app/\(tests\)/useUrlState/state.ts
# Check how it's used in components
echo "\nChecking Comp1.tsx implementation:"
cat packages/example-nextjs15/src/app/\(tests\)/useUrlState/Comp1.tsx
echo "\nChecking layout.tsx implementation:"
cat packages/example-nextjs15/src/app/\(tests\)/useUrlState/layout.tsx
Length of output: 1793
packages/example-nextjs14/src/app/Status.tsx (2)
36-36: LGTM! Improved variable naming.
The rename from state to urlState enhances code clarity by explicitly indicating the state's URL-based nature.
17-20: LGTM! Verify pattern consistency across codebase.
The updated useUrlState hook usage correctly implements the new pattern, taking form as the first argument and options as the second argument.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
Pattern consistency verified across codebase
The codebase shows consistent usage of the new pattern where useUrlState takes the default state as the first argument and options as the second argument. All instances follow this pattern, including:
- Example apps (nextjs14, nextjs15, react-router6)
- Documentation (README files)
- Test components
- Implementation files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useUrlState hook
# Expect: All occurrences should follow the new pattern
# Search for useUrlState usage patterns
rg -A 2 "useUrlState\("
Length of output: 8249
packages/example-react-router6/src/Status-for-test.tsx (2)
6-6: LGTM! Hook usage simplified and improved.
The updated hook usage with direct state passing aligns well with the PR's objective of streamlining the API. The variable rename to urlState is also more semantically clear.
20-20: LGTM! Consistent variable usage.
The JSON display correctly uses the renamed urlState variable, maintaining consistency with the hook changes.
packages/example-nextjs15/src/app/Status-for-test.tsx (2)
21-21: LGTM! Variable usage updated consistently.
The JSON output correctly uses the renamed urlState variable, maintaining consistency with the hook usage change above.
7-7: LGTM! Hook usage updated correctly.
The useUrlState hook implementation has been properly updated to use the new parameter structure, with form as the first argument and options as the second argument.
Let's verify consistent usage across the codebase:
✅ Verification successful
All usages of useUrlState hook follow the new pattern correctly
The verification shows that all instances of useUrlState across the codebase follow the correct pattern where:
- The first argument is the state object (form, stateShape, etc.)
- The second argument is an optional options object containing configuration like searchParams, replace, etc.
The implementation in Status-for-test.tsx is consistent with this pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of useUrlState hook
# Expect: All occurrences should follow the new pattern
# Search for useUrlState usage patterns
rg "useUrlState\(" --type ts --type tsx
Length of output: 69
Script:
#!/bin/bash
# Search for useUrlState usage patterns without file type restrictions
# since the previous attempt had issues with tsx extension
rg "useUrlState\(" -A 2
Length of output: 8249
packages/example-nextjs14/src/app/Status-for-test.tsx (1)
21-21: LGTM! Variable update is consistent.
The JSON display correctly reflects the renamed state variable from the hook.
packages/example-nextjs14/src/app/components/CodeBlocksRR.tsx (3)
2-2: LGTM: Import statement updated to reflect new component name.
The change from CodeBlockForm to CodeBlockState better reflects the component's purpose of demonstrating state management.
11-11: LGTM: Component usage updated consistently.
The component usage has been updated to match the new import, maintaining consistency throughout the file.
46-46: LGTM: Hook usage simplified appropriately.
The simplified usage of useUrlState provides a good contrast with ComponentA's example, demonstrating both basic and advanced usage patterns.
packages/example-nextjs14/src/app/components/CodeBlocksNext.tsx (3)
1-2: LGTM: Import changes align with component updates.
The replacement of CodeBlockForm with CodeBlockState is consistent with the broader changes across the codebase.
11-11: LGTM: Component replacement is consistent.
The CodeBlockState component is appropriately used in the quick start guide section.
52-52: LGTM: Clear demonstration of searchParams usage.
The example effectively shows how to integrate server-side searchParams with the useUrlState hook using the new parameter structure.
packages/example-nextjs14/src/app/Form.tsx (1)
18-20: LGTM! Clean and intuitive hook usage.
The updated useUrlState hook usage with form as the first argument and options as the second argument provides a more natural API design. This matches common React patterns where the primary argument (state) comes first, followed by configuration options.
packages/example-nextjs15/src/app/Form-for-test.tsx (1)
16-19: LGTM! Verify type safety of the form object.
The new hook initialization aligns with the PR objectives, using the first argument for the default state and the second for options.
✅ Verification successful
Type safety verified - implementation is correct
The form object is properly typed with Form interface that matches all the component's usage:
nameas string (used in text input)ageas optional number (used in number input)agree_to_termsas boolean (used in checkbox)tagsas array with properly typedid,text,time, and optionalisofields (used in tags section)
The useUrlState hook usage is type-safe as it correctly handles all these fields in the component's implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the type definition of the form object
ast-grep --pattern 'export const form = $_'
Length of output: 45
Script:
#!/bin/bash
# Let's try to find the form declaration with ripgrep for better coverage
rg "const form\s*=\s*" -A 5 packages/example-nextjs15/src/app/Form-for-test.tsx
Length of output: 81
Script:
#!/bin/bash
# Let's first check if the file exists and see its content
cat packages/example-nextjs15/src/app/Form-for-test.tsx
Length of output: 4851
Script:
#!/bin/bash
# Let's check the form import from shared/form
rg "export const form" --type ts
Length of output: 912
Script:
#!/bin/bash
# Let's check the form type definition in shared/form.ts
cat packages/shared/form.ts
Length of output: 281
🧰 Tools
🪛 Biome
[error] 18-18: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/example-nextjs14/src/app/Form-for-test.tsx (4)
Line range hint 46-82: LGTM! Well-structured event handlers.
The event handlers are properly memoized and follow a consistent pattern for state updates.
🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
Line range hint 84-200: LGTM! Clean and well-structured rendering implementation.
The component's render logic is well-organized, making good use of shared components and maintaining consistent state management patterns.
🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
17-17: Verify alignment with other components using useUrlState.
The implementation correctly uses the first argument for defaultState as per PR objectives. Let's verify this pattern is consistent across other components.
#!/bin/bash
# Search for other useUrlState usages to ensure consistent implementation
rg "useUrlState\(" --type=typescriptLine range hint 24-45: Consider optimizing the test-related state updates.
The effect contains multiple consecutive state updates within the length check that could be consolidated to reduce renders. Also, consider adding appropriate dependencies to the effect's dependency array if the validations should run on urlState changes.
🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/example-react-router6/src/Form-for-test.tsx (2)
9-11: LGTM! API change aligns with PR objectives.
The simplified useUrlState hook initialization improves readability by directly passing the form state as the first argument, followed by options.
🧰 Tools
🪛 Biome
[error] 10-10: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
9-11: Verify consistent implementation across components.
The AI summary indicates similar changes in other components. Let's verify the consistency of the useUrlState hook usage pattern.
✅ Verification successful
Based on the search results, I can now generate the final response:
The useUrlState hook implementation is consistent across the codebase
The usage pattern of useUrlState with the replace option is consistent across components:
- In Form-for-test.tsx files (both Next.js and React Router), the
replaceoption is dynamically set based on the URL parameter:sp.get("replace") === "false" ? false : true - Other components either explicitly set
replace: false/trueor omit it to use the default value - The hook's return values (
urlState,setState,setUrl) are consistently destructured based on each component's needs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all useUrlState hook usages to verify consistent implementation
rg -l 'useUrlState\(' | xargs rg -A 2 'useUrlState\('
Length of output: 8279
🧰 Tools
🪛 Biome
[error] 10-10: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
README.md (3)
168-168: LGTM! Example aligns with new API.
The example correctly demonstrates the new API where the first argument is used as the default state.
224-224: LGTM! Complex state example is well structured.
The example effectively demonstrates the new API with a complex state shape, including proper type inference.
323-323: LGTM! SSR example demonstrates proper usage.
The example correctly shows how to use the new API with SSR, where form is the first argument (default state) and { searchParams } is passed as the second argument (options).
packages/urlstate/next/useUrlState/useUrlState.ts (1)
110-113: Verify the merging order of options to prevent unintended overrides
When merging options in setUrl, the order of spreading objects affects which values take precedence. Ensure that user-provided options override both _opts and defaultOptions.
Run the following script to confirm the merge order:
✅ Verification successful
The results show that the merge order in both implementations (next and react-router packages) follows the same pattern: base options first (defaultOptions/defaultOpts), then internal options (_opts), and finally user-provided options (options). This is the correct order as it ensures that user-provided options take precedence over both default and internal options.
The options merging order is correctly implemented ✓
The current implementation follows the principle of increasing specificity where:
defaultOptionsprovides the base configuration_optsadds package-specific overridesoptions(user-provided) gets the final say
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the order of options merging in setUrl.
# Expectation: Options should be merged with user-provided options taking precedence.
ast-grep --pattern $'updateUrlBase($_, { ...$_, ...$_, ...$_ } )'
Length of output: 325
packages/urlstate/react-router/useUrlState/useUrlState.ts (1)
24-24:
Correct parameter destructuring syntax
The destructuring assignment defaultState: T is incorrect. You're attempting to assign the value of defaultState to the type parameter T, which is invalid. Instead, you should use defaultState without type annotation.
Apply this diff to fix the syntax error:
-export function useUrlState<T extends JSONCompatible>({
- defaultState: T,
+export function useUrlState<T extends JSONCompatible>({
+ defaultState,Likely invalid or redundant comment.
asmyshlyaev177
force-pushed
the
adjust_params
branch
from
f218109 to
b896d5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
packages/example-nextjs14/src/app/Form-for-test.tsx (1)
19-19: Simplify the boolean expression.The current ternary expression can be simplified by negating the condition directly.
- replace: sp.get('replace') === 'false' ? false : true, + replace: sp.get('replace') !== 'false',🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
packages/urlstate/next/useUrlState/useUrlState.ts (4)
15-19: Enhance the deprecation message with migration details.The deprecation message could be more helpful by including:
- The version when this will be removed
- A code example showing how to migrate from old to new format
-/** - * @deprecated . - * * use format `useUrlState(defaultState, { ...otherParams })` - * - */ +/** + * @deprecated Since version X.Y.Z. Will be removed in version A.B.C. + * Migrate from: + * ```ts + * useUrlState({ defaultState: form, searchParams }) + * ``` + * to: + * ```ts + * useUrlState(form, { searchParams }) + * ``` + */
39-48: Add TypeScript type annotations to documentation examples.The examples are clear but could be more helpful with explicit type annotations.
-export const form = { name: '', age: 0 }; -const { urlState, setState, setUrl } = useUrlState(form); +interface FormState { + name: string; + age: number; +} +export const form: FormState = { name: '', age: 0 }; +const { urlState, setState, setUrl } = useUrlState<FormState>(form);
72-92: Simplify parameter handling logic for better readability.The current implementation uses multiple ternary operations and type checks, which could be simplified.
Consider refactoring to:
function getParams<T>( defaultState: T | OldParams<T>, params?: Params ): { state: T; searchParams?: object; useHistory?: boolean; opts: { scroll?: boolean; replace?: boolean }; } { if ("defaultState" in defaultState) { const { defaultState: state, searchParams, useHistory, scroll, replace } = defaultState; return { state, searchParams, useHistory, opts: { scroll, replace }, }; } return { state: defaultState, searchParams: params?.searchParams, useHistory: params?.useHistory, opts: { scroll: params?.scroll, replace: params?.replace, }, }; }
186-199: Add JSDoc comments to interface definitions.The interfaces would benefit from documentation explaining each property.
/** * @deprecated Use Params interface instead * @interface OldParams * @template T - The type of the state object */ interface OldParams<T> { /** The initial state object */ defaultState: T; /** Optional search parameters from the URL */ searchParams?: object; /** Whether to use replace instead of push for navigation */ replace?: boolean; /** Whether to scroll to top after navigation */ scroll?: boolean; /** Whether to use window.history for navigation */ useHistory?: boolean; } /** * Configuration parameters for useUrlState hook * @interface Params */ type Params = { /** Optional search parameters from the URL */ searchParams?: object; /** Whether to use replace instead of push for navigation */ replace?: boolean; /** Whether to scroll to top after navigation */ scroll?: boolean; /** Whether to use window.history for navigation */ useHistory?: boolean; };packages/urlstate/react-router/useUrlState/useUrlState.ts (3)
18-35: Enhance deprecation notice with migration guidance.The deprecation notice should include more details about why the change was made and how to migrate to the new format.
/** - * @deprecated . - * * use format `useUrlState(defaultState, { ...otherParams })` + * @deprecated This format is deprecated and will be removed in the next major version. + * Please migrate to the new format: + * - Old: useUrlState({ defaultState: value, ...options }) + * - New: useUrlState(value, { ...options }) + * + * This change simplifies the API and makes it more intuitive to use. */
70-87: Simplify parameter processing logic.The current implementation has some code duplication and verbose type checking. Consider refactoring for better maintainability.
- const _defaultState = ( - "defaultState" in defaultState ? defaultState.defaultState : defaultState - ) as T; - const _useHistory = ( - "defaultState" in defaultState - ? defaultState.useHistory - : params?.useHistory - ) as boolean; - const _opts = - "defaultState" in defaultState - ? { - replace: defaultState.replace as boolean, - preventScrollReset: defaultState.preventScrollReset as boolean, - } - : { - replace: params?.replace, - preventScrollReset: params?.preventScrollReset as boolean, - }; + const isLegacyFormat = "defaultState" in defaultState; + const _defaultState = (isLegacyFormat ? defaultState.defaultState : defaultState) as T; + const _useHistory = isLegacyFormat ? defaultState.useHistory : params?.useHistory; + const _opts = { + replace: isLegacyFormat ? defaultState.replace : params?.replace, + preventScrollReset: isLegacyFormat + ? defaultState.preventScrollReset + : params?.preventScrollReset, + };
92-100: Optimize router initialization.The router initialization logic could be optimized in two ways:
- The dependency array for useMemo should include
_useHistory- The router object creation could be simplified
const router = React.useMemo( () => - _useHistory - ? routerHistory - : { - replace: (url: string, options: NavigateOptions) => - navigate(url, { ...defaultOpts, ..._opts, ...options }), - push: (url: string, options: NavigateOptions) => - navigate(url, { ...defaultOpts, ..._opts, ...options }), - }, + _useHistory ? routerHistory : { + replace: (url, options) => navigate(url, { ...defaultOpts, ..._opts, ...options }), + push: (url, options) => navigate(url, { ...defaultOpts, ..._opts, ...options }), + }, - [navigate, _opts], + [navigate, _opts, _useHistory], );
📜 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 (17)
README.md(5 hunks)packages/example-nextjs14/src/app/(tests)/useUrlState/Comp1.tsx(2 hunks)packages/example-nextjs14/src/app/Form-for-test.tsx(1 hunks)packages/example-nextjs14/src/app/Form.tsx(1 hunks)packages/example-nextjs14/src/app/Status-for-test.tsx(2 hunks)packages/example-nextjs14/src/app/Status.tsx(2 hunks)packages/example-nextjs14/src/app/components/CodeBlockState.tsx(1 hunks)packages/example-nextjs14/src/app/components/CodeBlocksNext.tsx(4 hunks)packages/example-nextjs14/src/app/components/CodeBlocksRR.tsx(4 hunks)packages/example-nextjs15/src/app/(tests)/useUrlState/Comp1.tsx(2 hunks)packages/example-nextjs15/src/app/Form-for-test.tsx(1 hunks)packages/example-nextjs15/src/app/Status-for-test.tsx(2 hunks)packages/example-react-router6/src/FewComponents/Component.tsx(1 hunks)packages/example-react-router6/src/Form-for-test.tsx(1 hunks)packages/example-react-router6/src/Status-for-test.tsx(2 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(4 hunks)packages/urlstate/react-router/useUrlState/useUrlState.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/example-nextjs14/src/app/(tests)/useUrlState/Comp1.tsx
- packages/example-nextjs14/src/app/Form.tsx
- packages/example-nextjs14/src/app/Status-for-test.tsx
- packages/example-nextjs14/src/app/Status.tsx
- packages/example-nextjs14/src/app/components/CodeBlockState.tsx
- packages/example-nextjs14/src/app/components/CodeBlocksNext.tsx
- packages/example-nextjs15/src/app/(tests)/useUrlState/Comp1.tsx
- packages/example-nextjs15/src/app/Form-for-test.tsx
- packages/example-nextjs15/src/app/Status-for-test.tsx
- packages/example-react-router6/src/FewComponents/Component.tsx
- packages/example-react-router6/src/Form-for-test.tsx
- packages/example-react-router6/src/Status-for-test.tsx
🧰 Additional context used
🪛 Biome
packages/example-nextjs14/src/app/Form-for-test.tsx
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (12)
packages/example-nextjs14/src/app/components/CodeBlocksRR.tsx (4)
11-11: LGTM!
The component replacement is consistent with the import changes.
22-23: Great addition of documentation reference!
The example clearly demonstrates the new hook usage pattern and includes a helpful link to the documentation. The code shows both the basic usage and advanced options with replace: true.
46-46: LGTM! Clear demonstration of basic usage
This example effectively shows the simplified hook usage without additional options, providing a good contrast to the more complex example in ComponentA.
1-2: Verify component replacement impact
The switch from CodeBlockForm to CodeBlockState suggests a change in the state management approach. Let's verify that this component is properly implemented and all its consumers are updated.
✅ Verification successful
The previous search for CodeBlockForm returned no results, which is good as it indicates complete removal. However, we should verify the existence and proper implementation of CodeBlockState that replaced it.
Component replacement is properly implemented
The migration from CodeBlockForm to CodeBlockState has been completed successfully:
- No references to the old
CodeBlockFormcomponent remain in the codebase CodeBlockStatecomponent is properly implemented and exists in its own file- All consumers (
CodeBlocksRR.tsxandCodeBlocksNext.tsx) have been updated to use the new component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to CodeBlockForm to ensure complete migration
rg "CodeBlockForm"
Length of output: 18
Script:
#!/bin/bash
# Let's first check if the file exists and see its content
fd "CodeBlockState.tsx?" --type f --exec cat {}
# Also search for any usage of CodeBlockState
rg "CodeBlockState" -A 2
Length of output: 1845
packages/example-nextjs14/src/app/Form-for-test.tsx (1)
17-21: LGTM! Hook usage updated correctly.
The change aligns with the PR objective, simplifying the useUrlState hook usage by passing form directly as the first argument, followed by the options object.
Let's verify this pattern is consistently applied across the codebase:
✅ Verification successful
The hook usage pattern is consistent across the codebase
After reviewing all usages of useUrlState across the codebase, the pattern of passing the state/form as the first argument followed by an optional options object is consistently applied. The implementation in Form-for-test.tsx follows this established pattern correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other useUrlState usages to ensure consistent pattern
rg "useUrlState\(" --type tsx --type ts
Length of output: 69
Script:
#!/bin/bash
# Search for useUrlState usages with ripgrep, without file type filtering
rg "useUrlState\("
# Also try finding the files first
fd -e tsx -e ts
Length of output: 10860
🧰 Tools
🪛 Biome
[error] 19-19: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
packages/urlstate/next/useUrlState/useUrlState.ts (1)
59-71: Well-structured function signatures with good backward compatibility.
The new signatures are more intuitive and align with common React patterns while maintaining backward compatibility through function overloading.
packages/urlstate/react-router/useUrlState/useUrlState.ts (1)
Line range hint 135-185: LGTM: Well-documented return object structure.
The return object structure is well-organized with:
- Clear deprecation notices for old methods
- Comprehensive documentation with examples
- Proper type definitions
README.md (5)
168-168: LGTM! Clear demonstration of the new hook usage pattern.
The example correctly shows the simplified API where the state object is passed directly as the first argument, aligning with the PR objectives.
224-224: LGTM! Excellent demonstration of type-safe complex state management.
The example effectively shows how the hook handles complex state shapes while maintaining type safety and proper state updates.
323-323: LGTM! Clear demonstration of SSR integration.
The example properly shows how to use the hook with server-side rendering by passing searchParams from server components.
429-429: LGTM! Proper demonstration of React Router integration.
The example correctly shows how to use the hook with React Router while maintaining consistent API across different routing solutions.
543-543: LGTM! Excellent pattern for creating reusable state hooks.
The example effectively demonstrates how to create custom hooks for specific state slices while leveraging the new API.
asmyshlyaev177
force-pushed
the
adjust_params
branch
from
c11aef4 to
f064586
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
packages/urlstate/utils.ts (1)
98-99: Consider adding deep merge support.The current implementation performs a shallow merge, which might not handle nested objects correctly. Consider using a deep merge utility for better handling of complex objects.
Example implementation:
export function assignValue<T extends object>(shape: T, newVal: Partial<T>) { const result: T = JSON.parse(JSON.stringify(shape)); // Deep clone Object.entries(shape).forEach(([key]) => { const _key = key as keyof T; const valExists = newVal[_key] !== undefined; if (valExists) { if (typeof shape[_key] === 'object' && typeof newVal[_key] === 'object') { result[_key] = { ...shape[_key], ...newVal[_key] } as T[keyof T]; } else { result[_key] = newVal[_key] as T[keyof T]; } } }); return result; }packages/urlstate/utils.test.ts (1)
120-121: Consider updating the test description.The test case correctly verifies the value override behavior, but its description "should override curr value with newValue" doesn't match the new implementation where there's no explicit "curr" value anymore.
Consider renaming to "should override default value with new value" or similar to better reflect the current behavior.
packages/urlstate/next/useUrlState/useUrlState.ts (5)
15-34: Enhance the deprecation notice with migration guidance.The deprecation notice should include:
- When this API will be removed
- A code example showing how to migrate from the old to the new format
/** * @deprecated . - * * use format `useUrlState(defaultState, { ...otherParams })` + * @deprecated This format will be removed in the next major version. + * Migrate to the new format: + * + * Before: + * ```ts + * useUrlState({ defaultState: form, searchParams, replace: true }) + * ``` + * + * After: + * ```ts + * useUrlState(form, { searchParams, replace: true }) + * ``` */
40-49: Enhance API documentation with return value types.The documentation should include descriptions of return values and their types for better developer experience.
* @param {JSONCompatible<T>} [defaultState] Fallback (default) values for state * @param {Object} params - Object with other parameters * @param {?SearchParams<T>} params.searchParams searchParams from Next server component * @param {boolean} params.useHistory use window.history for navigation, default true, no _rsc requests https://github.com/vercel/next.js/discussions/59167 + * @returns {Object} Hook return values + * @returns {T} returns.urlState - Current state synchronized with URL + * @returns {function} returns.setState - Function to update state without URL changes + * @returns {function} returns.setUrl - Function to update both state and URL * * * Example:
73-93: Improve default value handling for optional parameters.The current implementation might lead to undefined behavior when partial params are provided. Consider using object destructuring with default values.
- const _opts = - "defaultState" in defaultState - ? { - scroll: defaultState.scroll as boolean, - replace: defaultState.replace as boolean, - } - : { scroll: params?.scroll, replace: params?.replace }; + const _opts = { + scroll: "defaultState" in defaultState ? defaultState.scroll : params?.scroll ?? defaultOptions.scroll, + replace: "defaultState" in defaultState ? defaultState.replace : params?.replace ?? defaultOptions.replace, + };
111-114: Optimize useCallback dependencies.The dependency array could be more specific by including only the required properties from
_opts.- [updateUrlBase, _opts], + [updateUrlBase, _opts.scroll, _opts.replace],
187-200: Add JSDoc comments to interface definitions.Consider adding descriptive JSDoc comments to improve code documentation.
+/** + * @deprecated Use Params interface instead + * @interface OldParams + * @template T - The type of the state object + */ interface OldParams<T> { defaultState: T; searchParams?: object; replace?: boolean; scroll?: boolean; useHistory?: boolean; } +/** + * Configuration parameters for useUrlState hook + * @interface Params + */ type Params = { searchParams?: object; replace?: boolean; scroll?: boolean; useHistory?: boolean; };packages/urlstate/react-router/useUrlState/useUrlState.ts (4)
18-23: Enhance deprecation notice with migration guidance.The deprecation notice could be more helpful by including a complete example of how to migrate from the old format to the new format.
/** * @deprecated . - * * use format `useUrlState(defaultState, { ...otherParams })` + * This format is deprecated and will be removed in the next major version. + * Please migrate to the new format: + * + * Old format: + * useUrlState({ defaultState: form, replace: true }) + * + * New format: + * useUrlState(form, { replace: true }) * * . */
Line range hint
42-65: Enhance function documentation with return value descriptions.The documentation could be improved by adding descriptions for each return value and including more comprehensive examples.
* @param {JSONCompatible<T>} [defaultState] Fallback (default) values for state * @param {Object} params - Object with other parameters * @param {NavigateOptions} params.NavigateOptions See type from `react-router-dom` * @param {boolean} params.useHistory use window.history for navigation + * @returns {Object} + * @returns {T} urlState - Current state object + * @returns {Function} setState - Updates state without updating URL + * @returns {Function} setUrl - Updates both state and URL * * Example:
71-78: Consider extracting parameter derivation logic.The parameter derivation logic could be moved to a separate function to improve readability and testability.
+const deriveParams = <T>( + defaultState: T | OldParams<T>, + params?: Params, +) => { + const _defaultState = ( + "defaultState" in defaultState ? defaultState.defaultState : defaultState + ) as T; + const _useHistory = ( + "defaultState" in defaultState + ? defaultState.useHistory + : params?.useHistory + ) as boolean; + return { _defaultState, _useHistory }; +}; + const _defaultState = ( "defaultState" in defaultState ? defaultState.defaultState : defaultState ) as T;
187-198: Add JSDoc comments to type definitions.The type definitions would benefit from documentation explaining the purpose of each property, especially
searchParams.+/** + * @deprecated Use the new parameter format with Params interface + */ type OldParams<T> = { defaultState: T; useHistory?: boolean; + /** @deprecated This property is not used in the current implementation */ searchParams?: object; replace?: boolean; } & NavigateOptions; +/** + * Configuration options for useUrlState hook + */ interface Params extends NavigateOptions { useHistory?: boolean; + /** @deprecated This property is not used in the current implementation */ searchParams?: object; replace?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
README.md(6 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(4 hunks)packages/urlstate/react-router/useUrlState/useUrlState.ts(3 hunks)packages/urlstate/utils.test.ts(1 hunks)packages/urlstate/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (8)
packages/urlstate/utils.ts (2)
98-99: LGTM! Type-safe implementation with proper partial updates.
The function correctly handles type safety using TypeScript generics and properly manages partial updates.
98-99: Verify the impact of removing the curr parameter.
The removal of the curr parameter from the function signature might be a breaking change. Please ensure all consumers of this function have been updated accordingly.
Let's verify the usage:
packages/urlstate/utils.test.ts (4)
106-110: LGTM! Test case properly validates value assignment.
The test case correctly verifies the assignValue function's ability to modify a specific property while maintaining other properties intact.
114-117: LGTM! Immutability test is well structured.
The test case properly verifies that assignValue maintains immutability by checking that neither input reference is returned.
124-125: LGTM! Critical test case for default value behavior.
This test case is particularly important as it validates that the shape values are correctly used as defaults when no override is provided, which aligns with the PR's objective of using the first argument for defaultState.
106-125: Test coverage remains comprehensive after parameter simplification.
The modified test suite maintains thorough coverage of the assignValue function's behavior after removing the curr parameter:
- Value assignment verification
- Immutability checks
- Override behavior
- Default value handling
The changes align well with the PR's objective of simplifying the defaultState handling while ensuring all critical functionality remains properly tested.
packages/urlstate/next/useUrlState/useUrlState.ts (1)
102-106: Past review comment is still applicable.
The previous suggestion about adding error handling for invalid URL parameters remains relevant.
packages/urlstate/react-router/useUrlState/useUrlState.ts (1)
Line range hint 120-132: Fix useEffect dependencies.
The effect has missing dependencies that could lead to stale closures.
use first argument for
defaultStateand second as object with other optionsPR title
Proposed Changes
Breaking Changes
Summary by CodeRabbit
Release Notes
New Features
useUrlStatehook for simplified usage, allowing direct state object initialization.Bug Fixes
useUrlStateimplementation.Documentation
README.mddetailing the new usage patterns and examples for theuseUrlStatehook.Refactor
useUrlStatehook effectively.