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: Equality check for non-primitives in clearOnDefault #504

Merged
merged 2 commits into from
Mar 2, 2024

Conversation

franky47
Copy link
Member

@franky47 franky47 commented Feb 22, 2024

As reported by @Kavan72 in #462 (comment)

Testing only for referential equality is not enough for types like arrays and objects.

This introduces a new property of the parsers to specify an equality function to use when comparing against the default value.

  • Add jsdoc to the Parser type to reflect when to specify the eq property.

Copy link

vercel bot commented Feb 22, 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 Mar 2, 2024 3:19pm

@Kavan72
Copy link

Kavan72 commented Feb 26, 2024

any update here ?

@Kavan72
Copy link

Kavan72 commented Mar 1, 2024

hey @franky47 is there any update on this ?

@franky47
Copy link
Member Author

franky47 commented Mar 1, 2024

@Kavan72 sorry I'm on holiday with sick wife and kids, kind of hard to free up time to work on this at the moment, hopefully I can merge this next week.

@Kavan72
Copy link

Kavan72 commented Mar 1, 2024

@franky47 Great, take your time and have good weekends 😊

@franky47 franky47 merged commit e8c9720 into next Mar 2, 2024
15 checks passed
@franky47 franky47 deleted the fix/clear-on-default-equality-check branch March 2, 2024 15:25
@franky47 franky47 mentioned this pull request Mar 2, 2024
Copy link

github-actions bot commented Mar 2, 2024

🎉 This PR is included in version 1.17.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Kavan72
Copy link

Kavan72 commented Mar 2, 2024

@franky47 I think one edge case is still missing here. Suppose my URL looks like this:

/example/?status=QUALIFIED,CONFIRMED,PENDING

and I'm changing this status with multiple checkboxes. Users can select any checkbox without any specific order. For example, if a user selects 'CONFIRMED' first, then 'PENDING', the URL would look like this:

/example/?status=CONFIRMED,PENDING,QUALIFIED

In this case, the state remains the same but clearOnDefault will not be triggered due to checking the data with index using the eq method:

a.every((value, index) => itemEq(value, b[index]!)):

here

a: ['QUALIFIED', 'CONFIRMED', 'PENDING']
b: ['CONFIRMED', 'PENDING', 'QUALIFIED']

and this condition is not met because of b[index]!. Therefore, the condition is not the same.

@franky47
Copy link
Member Author

franky47 commented Mar 2, 2024

For this, I'd say you could write your own version of parseAsArrayOf and specify your own logic in the eq property.

In some cases, the order matters, in some other it doesn't.

@Kavan72
Copy link

Kavan72 commented Mar 2, 2024

Okay perfect 👍

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.

2 participants