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

[FEATURE]: Allow to pass empty values to inArray #1295

Open
nounderline opened this issue Sep 25, 2023 · 8 comments · May be fixed by #1664 or #2502
Open

[FEATURE]: Allow to pass empty values to inArray #1295

nounderline opened this issue Sep 25, 2023 · 8 comments · May be fixed by #1664 or #2502
Assignees
Labels
enhancement New feature or request

Comments

@nounderline
Copy link

Describe what you want

Passing empty array to inArray throws:

inArray requires at least one value

I think it would be more ergonomic to allow empty array as it would avoid additional checks outside of query builder.

Also, throwing an error causes unexpected runtime error if the array changes dynamically.

That's actually what have happened to me with following code:

const topPosts = await DB.select(posts.text, posts.mentions).from(posts)
// following will throw if there happen to be no mentions in dev database, but may surely be on production
// causing unexpected runtime error
const mentionedPeople = DB.select().from(people).where(inArray(people.id, posts.flatMap(v => v.mentions))

doHeavyweightCalculationsUnsuitableToADatabase(topPosts, mentionedPeople)
@bpinney
Copy link

bpinney commented Dec 15, 2023

I believe such a change would lead to an easier developer experience. That being said, similar changes to other aspects of the API have been closed by the team with no action.

See #1078

@sys13
Copy link
Sponsor

sys13 commented Dec 15, 2023

I would expect notInArray with an empty array to basically be as if there was no filter at all. It is true for every row that they don't match any elements in the array.

@Angelelz
Copy link
Collaborator

Angelelz commented Dec 15, 2023

I believe such a change would lead to an easier developer experience. That being said, similar changes to other aspects of the API have been closed by the team with no action.

See #1078

I'm not part of the team. I'm just trying to help cleanup the issues.

There are two options that have been discussed:

  1. Make inArray and notInArray accept a tuple with at least one value, ie: [T, ...T[]]. That will make sure the user has to be certain that the array has at least one value. The user will probably have to implement something like this to please the compiler:
function isTuple<T extends any[]>(array: T): T is [T, ...T[]] {
   return array.length > 0;
}

And handle the case accordingly in userland.

  1. Include the array.length > 0 in the inArray and noInArray functions so that they return false and true respectively.

The down side of the first is that the user has to deal with it (like they are right now, just now the types help them figure it out they need to do it), and handle the case when the array is empty, like this for example:

...
.where(isTuple(myArray) ? inArray(column, myArray) : sql`false`)
...

The down side of the second one is that it introduces hidden behavior, we just don't know down the line what other issues it will start.

@sys13
Copy link
Sponsor

sys13 commented Dec 15, 2023

I think it's reasonable for a user to assume the following functionality:

  • inArray with an empty array would filter out all values
  • notInArray with an empty array would keep all values

In JS: [].includes('hi') -> false, which is consistent with the above proposed functionality. The functionality does not prevent a developer from checking for an empty array if they would so choose. Forcing them to check is the current state as notInArray with an empty array results in a runtime error (which I was not expecting and did not get a type error for), but also leads to needing more code to check for empty arrays.

@rbuchberger
Copy link

Adding a +1 for this; throwing an error when passed an empty array is not useful nor expected, nor does it raise a typescript error which especially sucks.

@mikkelwf
Copy link

I've been using inArray extensively in my recent project and handling this behavior is my single biggest pain point with drizzle.

Throwing an error because the array is empty is not expected at all, and I've spent a fair amount of time fixing production code just because of this "feature".

I honestly don't see why (as a benefit to the developer) the empty array is not handled internally by drizzle and that it is expected that this should be handled externally outside drizzle instead.

@DenisBessa
Copy link

This could be a solution: https://moderndash.io/docs/arrayminlength

@RemiPeruto RemiPeruto linked a pull request Jun 13, 2024 that will close this issue
@RemiPeruto
Copy link

Hello guys !
Since @Angelelz seems to be busy I recreated a merge request to close this issue.

Do not hesitate to add your thumb-ups ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
8 participants