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

Empty query not returning expected value #648

Open
fedediaz-deliverect opened this issue Sep 25, 2024 · 10 comments
Open

Empty query not returning expected value #648

fedediaz-deliverect opened this issue Sep 25, 2024 · 10 comments
Labels
duplicate This issue or pull request already exists parsers/built-in Related to built-in parsers

Comments

@fedediaz-deliverect
Copy link

fedediaz-deliverect commented Sep 25, 2024

Context

What's your version of nuqs?

"nuqs": "^1.19.2"

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

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.1.0: Mon Oct  9 21:32:11 PDT 2023; root:xnu-10002.41.9~7/RELEASE_ARM64_T6030
  Available memory (MB): 18432
  Available CPU cores: 11
Binaries:
  Node: 20.14.0
  npm: 10.7.0
  Yarn: 1.22.22
  pnpm: 8.15.5
Relevant Packages:
  next: 14.2.10 // There is a newer version (14.2.13) available, upgrade recommended!
  eslint-config-next: 14.2.4
  react: 18.3.1
  react-dom: 18.3.1
  typescript: 5.3.3
Next.js Config:
  output: standalone
 ⚠ There is a newer version (14.2.13) available, upgrade recommended!
   Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
   Read more - https://nextjs.org/docs/messages/opening-an-issue

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

Based on the documentation:
Screenshot 2024-09-25 at 14 25 25

When having an array of strings define like this in the useQueryState:

const [ids, setIds] = useQueryState(
   "ids"
    parseAsArrayOf(parseAsString).withOptions({ shallow: false, scroll: false, clearOnDefault: true }),
  );

And a query like this in the browser:
?ids=

I expect ids to be:
['']

instead I receive:
[]

Screenshot 2024-09-25 at 14 20 07

this happens because of (https://github.com/47ng/nuqs/issues/329). But in my opinion this should have never been approved since at the moment there's no way to represent empty value.

Possible solution can be have 2 parsers, one called parseAsNonEmptyArrayOf (name is an example, i'm sure there is a better name for it), and that returns "" if query is there but no value is there

@fedediaz-deliverect fedediaz-deliverect added the bug Something isn't working label Sep 25, 2024
@imMatheus
Copy link

IMO this behavior makes more sense. Its empty then its empty. the example of /?name= returning "" makes sense as that is the "empty" state of a string, where as for your case you say its should be a string array and the "empty" state for an array is just []. If you had an empty number array, by your logic it should then be [0]? Which does not make super much sense

@franky47 franky47 removed the bug Something isn't working label Sep 25, 2024
@franky47
Copy link
Member

Yes, this was the original behaviour, and after discussion in #329 we changed it to an empty array.

@franky47 franky47 added the duplicate This issue or pull request already exists label Sep 25, 2024
@fedediaz-deliverect
Copy link
Author

IMO this behavior makes more sense. Its empty then its empty. the example of /?name= returning "" makes sense as that is the "empty" state of a string, where as for your case you say its should be a string array and the "empty" state for an array is just []. If you had an empty number array, by your logic it should then be [0]? Which does not make super much sense

I understand it make some sense, but it doesn't represent the true state of the url because /?name=,123 will return [123].

@imMatheus
Copy link

IMO this behavior makes more sense. Its empty then its empty. the example of /?name= returning "" makes sense as that is the "empty" state of a string, where as for your case you say its should be a string array and the "empty" state for an array is just []. If you had an empty number array, by your logic it should then be [0]? Which does not make super much sense

I understand it make some sense, but it doesn't represent the true state of the url because /?name=,123 will return [123].

Okej yeah that is a bit of an annoying edge case, I feel your pain 😔 Maybe it should be some middle ground? Like an option that can be passed in so that it can be used for both cases?

@franky47
Copy link
Member

There are two different cases here:

  1. /?name=, empty value, IMHO should resolve to an empty array, as per Setting array state with empty array results in array with empty string #329.
  2. /?name=,123 resolving to [123] does feel like a bug. For a parseAsArrayOf(parseAsInteger.withDefault(0)), it should resolve to [0, 123]. This is a todo point in the parseAsArrayOf parser: handling default values of item parsers, rather than stripping null/undefined values.

For point n°1, if you wish to have a different behaviour, there's always the option to fork the code of the parseAsArrayOf parser into a custom one for your application, using createParser.

For point n°2, PRs are welcome.

@franky47 franky47 reopened this Sep 25, 2024
@fedediaz-deliverect
Copy link
Author

for point 1 that's what I did. copy paste the implementation you had for parseAsArrayOf and modify it to be as it was before. I still use the current implementation, don't get me wrong, it's just that it might be confusing given that it the docs it says to expect '' when /?name= .
for point 2, I know ;)

@imMatheus
Copy link

I fully agree with point 1, the current behavior makes sense.

And for point 2, I would be down to make a PR. The behavior would be that /?name=,123 should resolve ["", 123], with parseAsArrayOf(parseAsString) and should resolve [0, 123] for parseAsArrayOf(parseAsInteger.withDefault(0))? Just so that there is no confusion here :)

@franky47
Copy link
Member

@imMatheus I think the following might make sense, if we follow the behaviour of individual parsers:

// URL: ?name=,123

const [str]    = useQueryState('name', parseAsArrayOf(parseAsString))
const [strDef] = useQueryState('name', parseAsArrayOf(parseAsString.withDefault('foo')))
const [int]    = useQueryState('name', parseAsArrayOf(parseAsInteger))
const [intDef] = useQueryState('name', parseAsArrayOf(parseAsInteger.withDefault(42)))

// str:    [null, '123']
// strDef: ['foo', '123']
// int:    [null, 123]
// intDef: [42, 123]

Introducing nulls is not great, but if we don't have a default value, I don't see how we can do otherwise, aside from stripping them off the array.

Typing this correctly is going to be fun 🙃

@imMatheus
Copy link

imMatheus commented Sep 25, 2024

@imMatheus I think the following might make sense, if we follow the behaviour of individual parsers:

// URL: ?name=,123

const [str]    = useQueryState('name', parseAsArrayOf(parseAsString))
const [strDef] = useQueryState('name', parseAsArrayOf(parseAsString.withDefault('foo')))
const [int]    = useQueryState('name', parseAsArrayOf(parseAsInteger))
const [intDef] = useQueryState('name', parseAsArrayOf(parseAsInteger.withDefault(42)))

// str:    [null, '123']
// strDef: ['foo', '123']
// int:    [null, 123]
// intDef: [42, 123]

Introducing nulls is not great, but if we don't have a default value, I don't see how we can do otherwise, aside from stripping them off the array.

Typing this correctly is going to be fun 🙃

Makes sense! Is it okej if I give it a stab in trying to make a PR for this behavior then? :)

@franky47
Copy link
Member

Let's see what the community thinks of it first: https://x.com/fortysevenfx/status/1838931586676318369

@franky47 franky47 added the parsers/built-in Related to built-in parsers label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists parsers/built-in Related to built-in parsers
Projects
None yet
Development

No branches or pull requests

3 participants