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: Serialization of default values in createSerializer #642

Merged
merged 2 commits into from
Sep 23, 2024

Conversation

hugotiger
Copy link
Contributor

@hugotiger hugotiger commented Sep 19, 2024

When using parsers in createSerializer it would not respect the clearOnDefault option properly when serializing, thus adding key-value pairs to the string that shouldn't be there.

An example:

const serialize = createSerializer({
    int: parseAsInteger.withOptions({ clearOnDefault: true }).withDefault(0)
})
const result = serialize({ int: 0 })
//    ^ Result is `?int=0` instead of empty string

Closes #641.

Copy link

vercel bot commented Sep 19, 2024

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

A member of the Team first needs to authorize it.

@hugotiger hugotiger changed the base branch from next to v2/release September 19, 2024 07:49
@hugotiger hugotiger force-pushed the fix/serialization-default-values branch from 1c1b460 to 978631a Compare September 19, 2024 07:58
@hugotiger hugotiger changed the title Fix/serialization default values fix: Serialization of default values in createSerializer Sep 19, 2024
@hugotiger hugotiger force-pushed the fix/serialization-default-values branch from 978631a to f6ca270 Compare September 19, 2024 08:04
Copy link

vercel bot commented Sep 19, 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 Sep 23, 2024 3:05pm

@franky47
Copy link
Member

Thanks! Could you change back the base branch to next for this one? It's not a breaking change, so it can be published on the v1 release line.

I pushed a fix for the types to let parsers know they can have a default value.

@hugotiger hugotiger force-pushed the fix/serialization-default-values branch from 59b2963 to 0d1351f Compare September 19, 2024 13:35
@hugotiger
Copy link
Contributor Author

Thanks! Could you change back the base branch to next for this one? It's not a breaking change, so it can be published on the v1 release line.

I pushed a fix for the types to let parsers know they can have a default value.

Awesome! Thanks for the help ✌️ Rebased now and updated the description of the PR.

@hugotiger hugotiger marked this pull request as ready for review September 19, 2024 13:38
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.

@hugotiger
Copy link
Contributor Author

One test is failing, see https://github.com/47ng/nuqs/actions/runs/10936702232/job/30378534527#step:7:90

I'm not able to reproduce it locally 🤔 Could it be some caching issue in the action? For context when I run pnpm run test it's all green.

@hugotiger
Copy link
Contributor Author

One test is failing, see https://github.com/47ng/nuqs/actions/runs/10936702232/job/30378534527#step:7:90

I'm not able to reproduce it locally 🤔 Could it be some caching issue in the action? For context when I run pnpm run test it's all green.

I checked again now and it seems as if the CI was running on some previous commit. Could you do a rerun?

@franky47 franky47 closed this Sep 23, 2024
@franky47 franky47 reopened this Sep 23, 2024
@franky47 franky47 merged commit a1aa096 into 47ng:next Sep 23, 2024
20 checks passed
@franky47
Copy link
Member

Thanks!

Copy link

🎉 This PR is included in version 1.19.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47 franky47 mentioned this pull request Sep 23, 2024
@hugotiger hugotiger deleted the fix/serialization-default-values branch September 23, 2024 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clearOnDefault doesn't work with createSerializer
2 participants