refactor: D1 API conformance

[dtbuchholz]

Summary

The D1 API changed, so our API is no longer compatible. This introduces some breaking changes since some of the statement & database methods' parameters, generic types, and returned types have to be altered for conformance purposes.

Details

For reference, these are the old APIs, and these are the new ones. The interfaces the statement methods must conform to are the following—notice the generic type T = unknown is now T = Record<string, unknown> in most of these:

// old
bind(...values: any[]): D1PreparedStatement;
first<T = unknown>(colName?: string): Promise<T>;
run<T = unknown>(): Promise<D1Result<T>>;
all<T = unknown>(): Promise<D1Result<T>>;
raw<T = unknown>(): Promise<T[]>;

// new
bind(...values: unknown[]): D1PreparedStatement;
first<T = unknown>(colName: string): Promise<T | null>;
first<T = Record<string, unknown>>(): Promise<T | null>;
run<T = Record<string, unknown>>(): Promise<D1Result<T>>;
all<T = Record<string, unknown>>(): Promise<D1Result<T>>;
raw<T = unknown[]>(): Promise<T[]>;

None of the database methods have changed, except for exec, which now has a custom return type:

// old
interface D1Result<T = unknown> {
  results?: T[];
  success: boolean;
  error?: string;
  meta: any;
}
exec<T = unknown>(query: string): Promise<D1Result<T>>;

// new
interface D1ExecResult {
  count: number;
  duration: number;
}
exec(query: string): Promise<D1ExecResult>;

So, a new ExecResult interface has been added for the exec method's return type, which includes the required count and duration as well as optional txn and results (see below).

From a DX perspective, the following changes should be noted. It wasn't clear if all of these changes were necessary as I was simply comparing and trying to go 1:1 to the new D1 API:

• Removed colName parameter for the statement methods, except first.
• Changed generic types for relevant statement methods to use T = Record<string, S> instead of T = S, where S = unknown.
• For exec, it no longer can use the same Result we had—a new ExecResult interface has been added, and a wrapExecResult properly formats the data for the exec return type.
  • This introduces one notable breaking change—since this interface only requires count and duration, the typical meta object cannot be required.
  • Thus, instead of a pattern for mutating queries like await meta.txn?.wait(), it looks like await txn?.wait().

How it was tested

Altered existing tests to take these changes into account. It primarily affected the exec method since it uses a new interface that doesn't contain meta.

Note: it looks like I also included a new test for getChainPollingController, which is more relevant to #36.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

[dtbuchholz]

Generic type change for API conformance.

Also, I wasn't sure if the lines I deleted were necessary with the addition of this method above, but I could be wrong so lmk if I should add any of those back in.

[dtbuchholz]

As discussed in #36, I've added back the overload that has colName: undefined. (I think I was a bit confused on how it worked, but it's much clearer now.)

After some testing, I also updated the generic to K extends keyof T & string = keyof T & string. Before, it was technically possible for K to be a non-string value, so I altered this to help enforce the D1 string requirement for colName.

[dtbuchholz]

Also, it looks like a test failed for @tableland/local due to the exec change, but I'll wait to clean that up to make sure these changes are the correct approach. What's weird is that I didn't touch the local package / update its deps at all, and the tests pass when I run them on my machine.

edit: resolved. does lerna automatically use whatever code is present in other packages? e.g., for local, I didn't change up its deps at all, but a local test that used the sdk package's exec no longer worked due to the SDK's breaking change. in other words, the published version for @tableland/sdk is the listed dependency for local, but the local tests were using the SDK changes from this PR.

[joewagner]

@dtbuchholz This looks pretty good so far. I have a PR with some suggested additions to make the SDK D1 and typescript all work together here: #65

[joewagner]

In regard to versioning.
I think these changes would indicate a major bump right? I'm not sure why d1-orm didn't do a major, but I guess since they are still version 0.x.x they can break things without a major bump

[joewagner]

does lerna automatically use whatever code is present in other packages? e.g., for local, I didn't change up its deps at all, but a local test that used the sdk package's exec no longer worked due to the SDK's breaking change. in other words, the published version for @tableland/sdk is the listed dependency for local, but the local tests were using the SDK changes from this PR.

I think that's how lerna is designed to work. If you need to have incompatible packages in the same commit I think you can pin an older version of sdk in the local package.lock, but I'm not sure.

[dtbuchholz]

I think these changes would indicate a major bump right? I'm not sure why d1-orm didn't do a major, but I guess since they are still version 0.x.x they can break things without a major bump

@joewagner correct, we'll need a major version. and also correct—the d1-orm maintainer said that pre-major version, they're just keeping all breaking changes as minor versions.

[dtbuchholz]

@joewagner oh actually, i have a question with this one (merged from your PR). the new API uses:

raw<T = unknown[]>(): Promise<T[]>;

what's the reason you used T = unknown over T = S[] (where S = unknown)? is it more flexible to how we implement the underlying logic or something like that?

[joewagner]

I didn't think about it much. I ran the tests and saw that there was a type mismatch, then I just copied the D1 types exactly as they are on the main branch and the tests passed. You probably have more context then me, what was the reason for T = S[] (where S = unknown)?

[joewagner]

Looks like it's defined in the constructor, that might work better? Feel free to switch it back.

[dtbuchholz]

ahh i see what you did re: the tests and 3rd party libs. nooow it makes sense!

and nre: the way in which we implement raw with T = S seems to work just fine, even though the D1 API expects T = S[]. i think it's due to our impl where we return Array<ValueOf<T>>, which will properly adhere to the D1 return type T[] because of the of ValueOf.

[dtbuchholz]

More on this below. It does a check on a passed arg to make sure it is a valid Options object with a valid PollingController. the rest of the logic is handled in the first impl.

[dtbuchholz]

I discovered that if you try and use the top-most overload for first() and pass your own opts, the compiler sees the opts as colName, which obv isn't correct. (still not 100% clear why this occurs...)

for example, running something like this would fail because the passed opts/controller was seen as the first param in the implementation's signature...so then, the extractColumn method would try and use the opts as the colName.

await stmt.first<{ counter: number; info: string }>({
  controller,
})

in other words, before this new new logic, it would see this:

colName
{
  controller: {
    signal: AbortSignal { aborted: false },
    abort: [Function: abort],
    interval: 1500,
    cancel: [Function: cancel],
    timeout: 60000
  }
}
opts
{}

and with the new logic, it'll properly set colName/opts and use them thereafter.

The base branch was changed.

[sanderpick]

awesome! great work.