Changeset View
Standalone View
web/database/utils/db-utils.js
- This file was added.
// @flow | |||||
function parseSQLiteQueryResult<T>(result: QueryExecResult): $ReadOnlyArray<T> { | |||||
ashoat: This is a rare case where we can prefer a non-readonly type. In general we want to be… | |||||
ashoatUnsubmitted Not Done Inline ActionsWhere is this type QueryExecResult defined? Is it built into Flow? If so, can you link where it is defined in the Flow lib? ashoat: Where is this type `QueryExecResult` defined? Is it built into Flow? If so, can you link where… | |||||
kamilAuthorUnsubmitted Done Inline Actions
Thanks for the explanation, makes sense
In D7101, I was in the middle of creating a stack and didn't set the parent diff before your review, my bad kamil: > This is a rare case where we can prefer a non-readonly type. In general we want to be… | |||||
const { columns, values } = result; | |||||
return values.map(rowResult => { | |||||
const row: any = Object.fromEntries( | |||||
ashoatUnsubmitted Not Done Inline ActionsWhy is this any? You should always expect your reviewers to call you out on this, so you should annotate your diffs ahead of time... otherwise it is a wasted review cycle ashoat: Why is this `any`? You should always expect your reviewers to call you out on this, so you… | |||||
kamilAuthorUnsubmitted Done Inline Actions
There is no easy way to check flow types in runtime as they don't exist there. On the keyserver, there is simply an Object type returned from the database.
I am not sure what is your point if you talking about the fact that I didn't add any explanation - that's fair, I thought it was obvious since it's runtime-related types, but that's a bad assumption from my side. If you're implying that that's a bad approach codewise, I have two alternatives:
type ClientDBDraftInfo = { +key: string, +text: string, }; function parseDraftResult(result: QueryExecResult): ClientDBDraftInfo[] { const { columns, values } = result; return values.map(rowResult => { const keyId = columns.indexOf('key'); const textId = columns.indexOf('text'); if (keyId < 0 || textId < 0) { throw new Error('Invalid result: missing columns'); } const key = rowResult[keyId]; if (typeof key !== 'string') { throw new Error('Invalid key type'); } const text = rowResult[textId]; if (typeof text !== 'string') { throw new Error('Invalid text type'); } return { key, text, }; }); } but I'm not convinced if this is really needed, I would rather leave it as it is or use 1 and add invariants on results when there will be needed. kamil: > Why is this `any`?
There is no easy way to check flow types in runtime as they don't exist… | |||||
columns.map((key, index) => [key, rowResult[index]]), | |||||
); | |||||
return row; | |||||
}); | |||||
} | |||||
// NOTE: sql.js has behavior that when there are multiple statements in query | |||||
// e.g. "statement1; statement2; statement3;" | |||||
// and statement2 will not return anything, the result will be: | |||||
// [result1, result3], not [result1, undefined, result3] | |||||
function parseSQLiteResult<T>( | |||||
rawResult: $ReadOnlyArray<QueryExecResult>, | |||||
): $ReadOnlyArray<$ReadOnlyArray<T>> { | |||||
return rawResult.map((queryResult: QueryExecResult) => | |||||
parseSQLiteQueryResult<T>(queryResult), | |||||
); | |||||
} | |||||
export { parseSQLiteResult }; |
This is a rare case where we can prefer a non-readonly type. In general we want to be "flexible" with the caller. It's more flexible to have read-only types for inputs, but more flexible to allow callers to do whatever they want with the return. Since we are constructing a brand new array in this function, it should be okay to allow the caller to do whatever they want