fix: return urlState as normal type, not readonly #23
Conversation
return `urlState` as normal type, not `readonly`
WalkthroughThe pull request includes extensive updates to the documentation and functionality of the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Outside diff range and nitpick comments (5)
packages/urlstate/useSharedState/README.md (1)
46-46: Enhance parameter type documentation.The type definition has been simplified, but it could be more explicit about partial updates.
Consider expanding the documentation:
-value: T | ((currState: T) => T) - New state value or a function that receives the current state and returns the new state. +value: T | Partial<T> | ((currState: T) => T) - New state value (full or partial) or a function that receives the current state and returns the new state.This better reflects the type shown in the Returns section above (line 20:
setState: T | Partial<T> | (T) => void).packages/urlstate/next/useUrlState/README.md (1)
Line range hint
1-67: Consider enhancing the documentation with type informationWhile the documentation is comprehensive, it could benefit from explicitly showing the TypeScript types for the returned state and functions. This would make it clearer to users how to properly type their implementations.
Consider adding type information like this:
interface UrlStateResult<T> { urlState: T; setState: (value: T | Partial<T> | ((curr: T) => T)) => void; setUrl: (value: T | Partial<T> | ((curr: T) => T), options?: { replace?: boolean; scroll?: boolean }) => void; }🧰 Tools
🪛 Markdownlint
16-16: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
packages/urlstate/react-router/useUrlState/README.md (1)
Line range hint
1-67: Consider adding mutation examples to clarify new behavior.While the documentation is comprehensive, it would be helpful to add examples demonstrating direct state mutations now that
urlStateis no longer readonly. This would help users understand the practical implications of this change.Consider adding an example like:
// Example of direct state mutation (now possible) const { urlState, setUrl } = useUrlState({ count: 0 }); urlState.count++; // This is now valid setUrl(urlState); // Update URL with modified state🧰 Tools
🪛 Markdownlint
16-16: Punctuation: ':'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
packages/urlstate/next/useUrlState/useUrlState.ts (1)
117-117: Changes align with PR objective but warrant documentation updates.The removal of readonly constraints from
urlStateandstatesuccessfully implements the PR objective. However, consider the following recommendations:
- Update the JSDoc comments to explicitly document that the state is now mutable
- Add a warning about avoiding direct mutations of the state object
Add the following JSDoc comment above the
urlStateproperty:+ /** + * The current state object. While mutable, direct mutations should be avoided. + * Use setState/setUrl for updates to ensure proper state management. + */ urlState: state,Also applies to: 121-121
packages/urlstate/react-router/useUrlState/useUrlState.ts (1)
123-127: Consider documenting state mutability implications.The removal of readonly constraints from
urlStateandstateimproves developer experience but introduces potential risks if state is modified directly instead of usingsetState/setUrl. While this aligns with React's standard patterns, it's important to ensure proper usage.Consider adding a note in the JSDoc comments above
urlStateto clarify:+ /** + * Current state object. While mutable, it's recommended to use setState/setUrl + * for updates to ensure proper URL synchronization. + */ urlState: state,
📜 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 (13)
README.md(0 hunks)packages/example-nextjs14/src/app/Form-for-test.tsx(0 hunks)packages/example-nextjs15/src/app/Form-for-test.tsx(0 hunks)packages/example-react-router6/src/Form-for-test.tsx(0 hunks)packages/urlstate/next/useUrlState/README.md(1 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(1 hunks)packages/urlstate/react-router/useUrlState/README.md(1 hunks)packages/urlstate/react-router/useUrlState/useUrlState.ts(1 hunks)packages/urlstate/useSharedState/README.md(2 hunks)packages/urlstate/useSharedState/useSharedState.ts(2 hunks)packages/urlstate/useUrlStateBase/README.md(1 hunks)packages/urlstate/useUrlStateBase/useUrlStateBase.ts(1 hunks)packages/urlstate/utils.ts(0 hunks)
💤 Files with no reviewable changes (5)
- README.md
- packages/example-nextjs14/src/app/Form-for-test.tsx
- packages/example-nextjs15/src/app/Form-for-test.tsx
- packages/example-react-router6/src/Form-for-test.tsx
- packages/urlstate/utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/urlstate/useUrlStateBase/README.md
🔇 Additional comments (8)
packages/urlstate/useSharedState/README.md (1)
17-18: Verify state property documentation completeness.
While removing "readonly" aligns with the PR objectives, consider adding a note about state mutability to help users understand the implications of this change.
Consider adding:
-state: T - The current state.
+state: T - The current state. Note: Unlike previous versions, this state object is mutable.packages/urlstate/next/useUrlState/README.md (1)
19-20: LGTM! Documentation accurately reflects the mutable nature of urlState
The removal of readonly designation aligns with the PR objectives and correctly represents the actual behavior of the urlState object.
packages/urlstate/react-router/useUrlState/README.md (1)
19-20: Documentation accurately reflects the type change.
The removal of the "readonly" designation from the urlState documentation aligns with the PR's objective to return urlState as a normal type instead of a readonly type. The documentation now correctly represents the mutable nature of the returned state object.
Let's verify the implementation matches this documentation change:
✅ Verification successful
Based on the implementation files examined, I can now generate the final response:
Documentation correctly reflects the implementation
The documentation change is accurate as the implementation in useUrlState.ts and useUrlStateBase.ts shows that:
- The
urlStateis returned as a regular object without readonly constraints - The type
T extends JSONCompatibleis used without any readonly modifiers - The state is mutable through
setStateandsetUrlfunctions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the TypeScript implementation to confirm urlState is no longer readonly
# Expected: No DeepReadonly type usage for urlState
# Search for any remaining readonly type declarations
rg -i "readonly.*urlState|DeepReadonly.*urlState" --type ts
# Search for the hook implementation
ast-grep --pattern 'export function useUrlState($_) {
$$$
}'
Length of output: 126
Script:
#!/bin/bash
# Let's try a different approach to find the implementation and type definitions
# Search for useUrlState implementation files
fd -e ts -e tsx useUrlState
# Once we find the files, let's examine their content
rg -l "useUrlState" --type=typescript
# Look for type definitions or interfaces related to urlState
rg "interface.*UrlState|type.*UrlState" --type=typescript
Length of output: 416
Script:
#!/bin/bash
# Now that we found the relevant files, let's examine their content
# Check the specific implementation for react-router useUrlState
cat packages/urlstate/react-router/useUrlState/useUrlState.ts
# Check the base implementation which might contain type definitions
cat packages/urlstate/useUrlStateBase/useUrlStateBase.ts
# Look for type definitions with corrected ripgrep syntax
rg "interface.*UrlState|type.*UrlState"
Length of output: 8120
packages/urlstate/useSharedState/useSharedState.ts (2)
5-5: LGTM! Import changes align with removing readonly types
The removal of the DeepReadonly import and addition of JSONCompatible type maintains type safety while allowing mutable state.
49-51: Type signature change looks good, verify type safety implications
The simplified type signature aligns with the PR objective of removing readonly constraints while maintaining support for both object and function-based updates.
Let's verify the impact on type safety across the codebase:
✅ Verification successful
Based on the gathered evidence, I can now generate the final response:
Type signature change is safe and correctly used throughout the codebase
The verification shows that:
- All setState usages follow immutable update patterns using spread operator (
...curr) - Function-based updates are consistently used to ensure state updates are based on current values
- No direct mutations of state objects are found in the codebase
- The simplified type maintains type safety while reducing unnecessary complexity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any TypeScript errors related to state mutations
# and verify usage patterns of useSharedState
# Search for useSharedState usage patterns
echo "Checking useSharedState usage patterns:"
rg -t typescript "useSharedState[<(]" -A 3
# Look for any mutation-related comments or TODOs
echo "Checking for mutation-related concerns:"
rg -t typescript "mutation|mutable|readonly" --glob "!*.test.*" --glob "!*.spec.*"
Length of output: 377
Script:
#!/bin/bash
# Let's try without typescript file type flag and use file extensions directly
echo "Checking useSharedState usage patterns:"
rg "useSharedState[<(]" --glob "*.ts" --glob "*.tsx" -A 3
echo -e "\nChecking for mutation-related concerns:"
rg "mutation|mutable|readonly" --glob "*.ts" --glob "*.tsx" --glob "!*.test.*" --glob "!*.spec.*"
# Also check for any type errors or issues in related files
echo -e "\nChecking for type-related files and interfaces:"
fd -e ts -e tsx | grep -i "type\|interface"
Length of output: 9140
Script:
#!/bin/bash
# Let's examine the implementation and usage of setState more closely
echo "Checking setState implementation:"
ast-grep --pattern 'setState = ($$$) => {
$$$
}'
echo -e "\nChecking actual state updates in examples:"
rg "setState\(" --glob "*.ts" --glob "*.tsx" -A 1
# Let's also check the type definitions
echo -e "\nChecking type definitions:"
cat packages/example-nextjs15/src/app/types.ts
Length of output: 8034
packages/urlstate/next/useUrlState/useUrlState.ts (1)
117-121: Verify impact of removing readonly constraint.
Let's check for any code that might be affected by this type change.
packages/urlstate/react-router/useUrlState/useUrlState.ts (1)
123-127: Verify impact of type change on existing code.
The change from readonly to mutable state might affect existing code that relies on type checking.
packages/urlstate/useUrlStateBase/useUrlStateBase.ts (1)
112-112:
Breaking Change: Removing readonly type constraint requires documentation
The removal of the DeepReadonly type constraint allows direct mutation of the state object. While this achieves the PR's objective, it has several implications:
- This is a breaking change for TypeScript users who may rely on type checking
- It deviates from React's immutable state pattern
- Could lead to bugs if state is accidentally mutated
Consider:
- Adding a comment in the JSDoc explaining why readonly was removed
- Documenting this breaking change in the changelog
- Adding a warning about direct mutations in the documentation
Let's check for any direct mutations in the codebase that might now be problematic:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
package.json (1)
106-106: Consider documenting the purpose of the pin-gh-deps script.The script appears to be a DevOps improvement for pinning GitHub Actions dependencies. While the functionality is clear from the command, adding a brief comment about its purpose would improve maintainability.
📜 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 (3)
package.json(2 hunks)packages/urlstate/next/useUrlState/useUrlState.ts(1 hunks)packages/urlstate/react-router/useUrlState/useUrlState.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/urlstate/next/useUrlState/useUrlState.ts
- packages/urlstate/react-router/useUrlState/useUrlState.ts
🔇 Additional comments (1)
package.json (1)
367-367: LGTM! DevOps dependency addition.
The pin-github-action dependency is correctly added with a stable version (^1.9.1) to support the new pin-gh-deps script.
Let's verify the workflow file exists:
✅ Verification successful
Perfect! The pin-github-action dependency is justified and properly used
The workflow file exists and contains multiple GitHub Actions that are already pinned to specific commit hashes, which is exactly what the pin-github-action package is designed to maintain. The presence of comments with version numbers (e.g., # v4.2.2) alongside commit hashes (e.g., @11bd71901bbe5b1630ceea73d27597364c9af683) indicates this pinning system is already in active use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the workflow file exists and contains GitHub Actions that can be pinned
# Expected: File exists and contains action references
if [ -f ".github/workflows/tests.yml" ]; then
echo "Workflow file exists"
# Look for action references (e.g., uses: action@version)
rg "uses: .+@" .github/workflows/tests.yml
else
echo "Workflow file not found"
fi
Length of output: 2574
asmyshlyaev177
deleted the
22-argument-of-type-readonly-string-is-not-assignable-to-parameter-of-type-string
branch
return
urlStateas normal type, notreadonlyfix #22
PR title
Proposed Changes
Breaking Changes
Summary by CodeRabbit
Release Notes
New Features
state-in-urllibrary, detailing capabilities like server-side rendering and framework flexibility.useUrlStateanduseSharedStatehooks.Bug Fixes
urlStateobject to comply with TypeScript's readonly constraints in various form components.Documentation
useUrlState,useSharedState, and other components, emphasizing state mutability and usage examples.Chores