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]: Don't throw when .values() is called with an empty array #1078

Closed
miketromba opened this issue Aug 19, 2023 · 3 comments
Closed
Labels
enhancement New feature or request

Comments

@miketromba
Copy link

miketromba commented Aug 19, 2023

Describe what you want

I recently converted my codebase over from Prisma to drizzle, and I have had multiple runtime bugs slip into production where I was passing in an empty array to insert.values() and getting this error: values() must be called with at least one value

image

The problem is, sometimes you need to dynamically create an array before passing it into the insert function,

const oldItems = [/* some retrieved array of items of unknown length */]
const newItems = oldItems.map(old => ({
    ...old,
    id: randomId()
}))

and it is very easy to forget to wrap the code in:

if(newItems.length){
    // perform insert
}

I propose allowing empty arrays to be passed in by simply resolving the promise with an early return instead of throwing an error. To me, this seems like the lesser of two evils.

@miketromba miketromba added the enhancement New feature or request label Aug 19, 2023
@miketromba miketromba changed the title [FEATURE]: Allow .values() to be called with an empty array [FEATURE]: Don't throw when .values() is called with an empty array Aug 19, 2023
@emdede
Copy link
Contributor

emdede commented Aug 23, 2023

👍 Would be great to also have this for inArray. It is work-aroundable, like OP said, but generates significant amount of noise in the code, and acts as potential source for runtime errors due to being a "bug" TS is not able to catch.

Fun fact: While I was checking my code for one of the usages to include here as an example, I did notice a bug because of exactly that.

        if (onlyIds.length) {
          await tx.insert(applicationServiceConnections).values(
            onlyIds.map((serviceId) => ({
              serviceId,
              applicationId: id,
            }))
          )
        }

        const serviceRows = await tx.query.services.findMany({
          with: {
            applications: true,
          },
          where: inArray(services.id, onlyIds),
        })

In the first block, I thought about the length check. But for the following query, I forgot it => runtime error.
If it could be made a TS Error, that would work for me, too.

@miketromba
Copy link
Author

@emdede Agreed, if it was possible to make it a TS error, that might be the ideal solution.

@Angelelz
Copy link
Collaborator

Angelelz commented Dec 1, 2023

It's hard to provide an API that everybody is happy with. Please see #441 for additional context.
I try to not make any db call without a try-catch because it's an over-the-wire request and errors do occur.
In your case it's a dynamic array so TS can't help either. I don't think returning early is a good option either because the return type will depend on the driver and if the query has a .returning() or not. It's will also be breaking change.
Maybe find a way to use a linter to detect this type of issues?
Please feel free to reopen if you have any other ideas.

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
Development

No branches or pull requests

3 participants