-
-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(type): decode
and encode
should be optional
#120
base: master
Are you sure you want to change the base?
Conversation
|
WalkthroughThe changes in this pull request focus on modifying the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for sveltekit-search-params ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/lib/sveltekit-search-params.ts (1)
391-391
: LGTM: Implementation aligned with flexible optionsThe change to
Partial<EncodeAndDecodeOptions<T>>
in the function implementation aligns well with the modifications in the overloads. It correctly handles cases whereencode
ordecode
might not be provided, falling back to the defaults fromDEFAULT_ENCODER_DECODER
.For improved clarity, consider adding a comment explaining the fallback behavior:
{ encode: encode = DEFAULT_ENCODER_DECODER.encode, decode: decode = DEFAULT_ENCODER_DECODER.decode, defaultValue, }: Partial<EncodeAndDecodeOptions<T>> = DEFAULT_ENCODER_DECODER, // ^ Fallback to DEFAULT_ENCODER_DECODER if no options providedThis change enhances flexibility and maintains consistency with the function overloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/lib/sveltekit-search-params.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/lib/sveltekit-search-params.ts (3)
377-377
: LGTM: Improved flexibility forqueryParam
optionsThe change from
EncodeAndDecodeOptions<T> & { defaultValue: T }
toPartial<EncodeAndDecodeOptions<T>> & { defaultValue: T }
is a good improvement. It allows users to omitencode
anddecode
functions when not needed, while still requiring adefaultValue
. This change enhances the function's usability without breaking existing functionality.
382-382
: LGTM: Consistent flexibility improvement for optionalqueryParam
The change from
EncodeAndDecodeOptions<T>
toPartial<EncodeAndDecodeOptions<T>>
in this overload is consistent with the changes made to the first overload. It allows all properties of the options object to be optional when nodefaultValue
is provided. This change maintains consistency across the function's overloads and improves overall flexibility.
Line range hint
377-391
: Summary: Successful implementation of optionalencode
anddecode
The changes to the
queryParam
function successfully implement the PR objective of making theencode
anddecode
options optional. This has been consistently applied across both function overloads and the main implementation. The modifications improve the flexibility of the function without breaking existing functionality, allowing users to omitencode
anddecode
when not needed.Key improvements:
- Enhanced usability by making options partial
- Maintained consistency across function overloads
- Proper fallback to default encoder/decoder when options are not provided
These changes will likely improve the developer experience when using the
queryParam
function, especially in cases where custom encoding/decoding is not required.
This pull request includes changes to the
src/lib/sveltekit-search-params.ts
file to make theEncodeAndDecodeOptions
parameter optional in thequeryParam
function. The most important changes involve modifying the type of theoptions
parameter toPartial<EncodeAndDecodeOptions<T>>
.Changes to function parameters:
src/lib/sveltekit-search-params.ts
: Modified theoptions
parameter in thequeryParam
function to be of typePartial<EncodeAndDecodeOptions<T>>
instead ofEncodeAndDecodeOptions<T>
. This change was applied to both overloads of the function.src/lib/sveltekit-search-params.ts
: Updated the destructuring assignment in thequeryParam
function to usePartial<EncodeAndDecodeOptions<T>>
for theencode
,decode
, anddefaultValue
properties.Summary by CodeRabbit
queryParam
function, allowing partial definitions of encoding and decoding options while requiring a default value.