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

clearOnDefault doesn't work in useQueryStates #618

Closed
dev-aly3n opened this issue Aug 30, 2024 · 7 comments · Fixed by #621
Closed

clearOnDefault doesn't work in useQueryStates #618

dev-aly3n opened this issue Aug 30, 2024 · 7 comments · Fixed by #621
Labels

Comments

@dev-aly3n
Copy link

dev-aly3n commented Aug 30, 2024

Context

What's your version of nuqs? ^1.17.7

Next.js information (obtained by running next info):

  next: 14.2.5 // There is a newer version (14.2.7) available, upgrade recommended!
  eslint-config-next: 14.1.0
  react: 18.2.0
  react-dom: 18.2.0
  typescript: 5.5.4
Next.js Config:
  output: N/A

Are you using:

  • ✅ The app router
  • ❌ The pages router
  • ❌ The basePath option in your Next.js config
  • ❌ The experimental windowHistorySupport flag in your Next.js config

Description

Hi, thanks fr this great package.
there is issue with clearing on default value when using useQueryStates and doesnt work correctly.

Reproduction

for reproduction try to use this hook with few key like below:

const [coordinates, setCoordinates] = useQueryStates(
  {
    lat: parseAsFloat.withDefault(45.18),
    lng: parseAsFloat.withDefault(5.72),
    temp:  parseAsString
        .withDefault("")
        .withOptions({ clearOnDefault: true })
  },
)

So, in this example, when you try to set one of the coordinates but keep the temp untouched, you can still see &temp= in the URL.

The expected behavior is to only include those with values other than the default ones in the URL.

@dev-aly3n dev-aly3n added the bug Something isn't working label Aug 30, 2024
@dev-aly3n
Copy link
Author

I see it doesnt work in that way however it let us to specify it (still an issue and I'm keeping this issue open to help maintainers).
the correct way is passing second object to the hook that contains all options.

const [coordinates, setCoordinates] = useQueryStates(
  {
    lat: parseAsFloat.withDefault(45.18),
    lng: parseAsFloat.withDefault(5.72),
    temp:  parseAsString
        .withDefault("")
  },
// the correct way to add option in useQueryStates
     {
        clearOnDefault: true,
      },
)

@franky47
Copy link
Member

franky47 commented Aug 30, 2024

Thanks for the report!

Yes, the global options should probably be merged with each parser's individual options, but this raises the question of which one should take precedence?

As for clearOnDefault, I'd say useQueryStates behaved correctly in the first case, as you are not "setting the default value" for temp, which would cause it to clear with that clearOnDefault option set. Not specifying a value means "leave it as it is", it's a partial set. I may have to update the docs to be clearer about this.

@dev-aly3n
Copy link
Author

Yes, updating the documentation will help a lot. Thank you.

I agree that clearOnDefault would be something global. However, for instance, I have 5 states, and 2 of them have history: "push". I don't want to push to history if the other 3 states change.

In this case, I had to create two separate useQueryStates: one containing the 2 states with pushing and the other containing the 3 remaining states.

@franky47
Copy link
Member

franky47 commented Aug 30, 2024

I have 5 states, and 2 of them have history: "push". I don't want to push to history if the other 3 states change.

So it seems the global options should be evaluated first, then any local per-parser options would take precedence, then any call-level options would take precedence over those, would that seem ok?

You could then do:

// Disclaimer: this code doesn't (yet) behave like this,
// it's an example of what option merging could look like.

const [{ lat, lng, temp, foo }, setSearchParams] = useQueryStates({
    lat: parseAsFloat.withDefault(45.18),
    lng: parseAsFloat.withDefault(5.72),
    temp:  parseAsString
        .withDefault("")
        .withOptions({ clearOnDefault: true }),
   foo: parseAsBoolean
        .withDefault(false)
        .withOptions({ history: 'push' })
  })
}, {
  // Global options
  scroll: false
})

// Only set lat & lng, history = replace, scroll = false
setSearchParams({ lat: 0, lng: 0 })

// Clear temp (set as default, equivalent to setSearchParams({ temp: null }))
// history = replace, scroll = false
setSearchParams({ temp: '' })

// Push foo with call-level options, history = push, scroll = false, shallow = false
setSearchParams({ foo: true }, { shallow: false })

@dev-aly3n
Copy link
Author

Yes, exactly. That would be great.

franky47 added a commit that referenced this issue Sep 1, 2024
Order of precedence (first non-nullish wins):
- call level
- parser level
- hook-global level (otherwise default).

Also fixes a bug with `clearOnDefault` in useQueryState where
a `true` top-level value could not be overriden by a call-level `false` value.

Closes #618.
franky47 added a commit that referenced this issue Sep 1, 2024
* fix: Allow parser-level options in useQueryStates

Order of precedence (first non-nullish wins):
- call level
- parser level
- hook-global level (otherwise default).

Also fixes a bug with `clearOnDefault` in useQueryState where
a `true` top-level value could not be overriden by a call-level `false` value.

Closes #618.
Copy link

github-actions bot commented Sep 2, 2024

🎉 This issue has been resolved in version 1.19.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Sep 2, 2024

🎉 This issue has been resolved in version 1.19.0 🎉

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants