Skip to content
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(serializer/types): Allow null value in serializer for parsers with default values #769

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

MartinCura
Copy link
Contributor

@MartinCura MartinCura commented Nov 15, 2024

See related discussion.

Passing null to the serialize of a search param whose parser has a default currently is a type error, even though the behaviour works as expected.

This is because for the values argument it infers the types using the same inferParserType which is fine for parsing but is not quite right for serializing. The way i'm fixing it is just declaring a different inferSerializerRecordType to be used here. If there's some TS-foo i don't know which allows reusing the same inferParserRecordType, that'd be even better!

CONTRIBUTING.md mentions "adding a minimal reproduction in the playground can be very helpful" but i couldn't immediately find how to create one there.

Added some very basic tests, feel free to point out if any more would be nice.

Copy link

vercel bot commented Nov 15, 2024

@MartinCura is attempting to deploy a commit to the 47ng Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, did you encounter errors with inferParserType that required splitting it? It should handle both a single parser and records.

packages/nuqs/src/serializer.ts Outdated Show resolved Hide resolved
Copy link

vercel bot commented Nov 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuqs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2024 0:57am

@franky47 franky47 added the deploy:preview Deploy a preview version of this PR on pkg.pr.new label Nov 15, 2024
@franky47 franky47 added this to the 🪵 Backlog milestone Nov 15, 2024
Copy link

pkg-pr-new bot commented Nov 15, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/nuqs@769

commit: 877d1d3

Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@franky47
Copy link
Member

Feel free to test the pkg.pr.new build to see if it works on your side.

@franky47 franky47 changed the title fix(serializer/types): allow null to serialize param whose parser has default fix(serializer/types): Allow null in serializer for parsers with default values Nov 15, 2024
@franky47 franky47 changed the title fix(serializer/types): Allow null in serializer for parsers with default values fix(serializer/types): Allow null value in serializer for parsers with default values Nov 15, 2024
@MartinCura
Copy link
Contributor Author

Feel free to test the pkg.pr.new build to see if it works on your side.

It does remove the type error ✅
image

I didn't see a changelog file so only thing i may be missing is being explicit about this in the docs, maybe, though not necessary.

Other than that, feel free to merge 🚀

@franky47 franky47 merged commit 84c5189 into 47ng:next Nov 15, 2024
31 checks passed
@franky47
Copy link
Member

The changelog is generated by semantic-release in the GitHub release when the package gets published.

I think we can omit the docs, this is the expected behaviour. Thanks again for your contribution! 🙏

@MartinCura MartinCura deleted the serialize_values_type branch November 15, 2024 13:03
Copy link

🎉 This PR is included in version 2.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy:preview Deploy a preview version of this PR on pkg.pr.new released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants