-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: D1 API conformance #63
Conversation
The D1 API changed such that the return type for `exec` is now `ExecResult`, which differs from the original API that returned a Restult<T>. This adds a new result type and wrapper, specifically, for the `exec` database method. D1 APIs can be seen here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts
The D1 API changed to where statement methods should now take a `T = Record<string, S>` over `T = unknown`, and no `colName` is needed anymore. This is true for all except for the `first` method, which does still allow for a `colName` to be passed with `T = unknown`. D1 APIs can be found here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts
colName: undefined, | ||
opts?: Options | ||
): Promise<T>; | ||
async first<T = Record<string, S>>(opts?: Options): Promise<T | null>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
edit: resolved. does lerna automatically use whatever code is present in other packages? e.g., for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
In regard to versioning. |
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 |
@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. |
packages/sdk/src/statement.ts
Outdated
@@ -266,7 +266,7 @@ export class Statement<S = unknown> { | |||
* @param opts An optional object used to control behavior, see {@link Options} | |||
* @returns An array of raw query results. | |||
*/ | |||
async raw<T = S[]>(opts: Options = {}): Promise<Array<ValueOf<T>>> { | |||
async raw<T = unknown>(opts: Options = {}): Promise<Array<ValueOf<T>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's defined in the constructor, that might work better? Feel free to switch it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -179,6 +180,11 @@ export class Statement<S = unknown> { | |||
} | |||
} | |||
|
|||
// Check if the first param seen by `first()` is an Options object | |||
#checkIsValidOpts(opts: any): opts is Options { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
opts: Options = {} | ||
): Promise<T | T[K] | null> { | ||
try { | ||
// Handle first overload to ensure passed `opts` are not set to `colName` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
In the top-most `first()` overload, when you pass `opts`, the implementation recognizes this as the `colName`. The private `checkIsValidOpts` method helps to check if the passed `colNameOrOpts` is indeed an Options object, and then it properly sets the `colName` and options accordingly before proceeding in the flow.
914596f
to
aa5f65c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! great work.
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 nowT = Record<string, unknown>
in most of these:None of the database methods have changed, except for
exec
, which now has a custom return type:So, a new
ExecResult
interface has been added for theexec
method's return type, which includes the requiredcount
andduration
as well as optionaltxn
andresults
(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:
colName
parameter for the statement methods, exceptfirst
.T = Record<string, S>
instead ofT = S
, whereS = unknown
.exec
, it no longer can use the sameResult
we had—a newExecResult
interface has been added, and awrapExecResult
properly formats the data for theexec
return type.count
andduration
, the typicalmeta
object cannot be required.await meta.txn?.wait()
, it looks likeawait 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 containmeta
.Note: it looks like I also included a new test for
getChainPollingController
, which is more relevant to #36.Checklist: