dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

feat: improve ergonomics of single result queries #189

Closed jsejcksn closed 2 years ago

jsejcksn commented 2 years ago

Ref: https://github.com/dyedgreen/deno-sqlite/issues/180

I'm opening this as a draft PR per our previous discussion.


I thought more about our discussion regarding backwards compatibility: it would also be backward-compatible to add a second options argument to query.one/oneEntry with an option named something like allowNone to indicate that null/undefined is an acceptable result when no matching row is found (instead of throwing an error). It feels cleaner to me to add some method overload signatures for better type resolution than to add additional public methods just to prevent needing to use try...catch statements. An options object also allows for future API expansion in a backward-compatible way, given thoughtful naming.

Note: My initial commit is a draft to demonstrate the concept: I didn't spend too much time thinking about names. Also, I prefer undefined over null in my own code for "empty", but null was suggested in the previous discussion, which is why I used it in the code in this PR. I'd like to hear your thoughts on this as well.

@dyedgreen Feedback on this approach?

dyedgreen commented 2 years ago

Hm, I'm still torn what would be the best approach here. There are four situations / things one might want to do:

  1. get exactly one row (throws when numRows != 1)
  2. get the first row (throws when numRows == 0)
  3. get exactly one row, if it exists (throws when numRows > 1, may return null)
  4. get the first row, if it exists (never throws, may return null)

All of these are possible with the current API, but the ergonomic ones are:

You can relatively easily get (2), by doing const [ first ] = query.all(); if (first == null) throw new Error();. And (3) requires you to do query.all(), check the length of the returned array, and then throw or return (or do the equivalent thing with iter, to be more efficient).

I quite like the API having one (read exactly one, case 1) and first (ready first or null, case 4). But that leaves two cases out. On the flip-side, the options argument reads awkwardly to me (especially when you have no parameters). Plus the function overloading makes it harder to understand the types. (But covering all three cases is pretty easy to do).

Something else we could try / think about: Having exactly(n, params), atLeast(n, params), and atMost(n, params), which would cover the above cases. (With one() being exactly(1) and maybe adding first as atLeast(1) for convenience?) The question is how to make the types work nicely but it seems like it would be possible.

andykais commented 2 years ago

Imo, null assertion checks belong outside this library. A developer could easily wrap their driver to perform their own checks. You dont know what error message will be useful to the user anyways, so even if you include a throw statement, itll likely be wrapped regardless, if throwing errors is a behavior that users want.

E.g. if deno-sqlite does not assert on empty, the following is pretty terse

const query = { some: 'query' }
const row = stmt.one(query)
if (!row) throw new Error(`row not found with query "${query}"`)

while if we do want to return empty rows, the following is ugly

try {
  return stmt.one(query)
} catch (e) {
  // yes we could create a `NotFoundError`, but thats still less ergonomic than returning empty rows
  if (e.message.includes('The query did not return any rows') return null
  else throw e
}

As far as safety goes, making the type signature of one return T | null should suffice to encourage safety by users. Also, a breaking change like this might be fine so long as its called out in the release. This library is still fairly new, so the chances that it is deeply integrated in a production system is not too likely yet.

dyedgreen commented 2 years ago

Closing this in favor of #206

jsejcksn commented 2 years ago

Closing this in favor of #206

Thanks; LGTM. I'm glad you changed your mind about this!

dyedgreen commented 2 years ago

(@jsejcksn this is now released)