Interactions-as-a-Service / d1-orm

A simple, strictly typed ORM, to assist you in using Cloudflare's D1 product
https://docs.interactions.rest/d1-orm/
Apache License 2.0
162 stars 11 forks source link

added insert or replace #41

Closed kingmesal closed 1 year ago

kingmesal commented 1 year ago

Updated tests and documentation to support providing Insert and replace with an added boolean parameter to InsertOne and InsertMany

changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: 9049500ce539b0f3ebce21798f6e37bc652990cd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------ | ----- | | d1-orm | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Skye-31 commented 1 year ago

Thanks for the PR! 3 things:

kingmesal commented 1 year ago

I missed the other question:

Do we want to allow people to set it per item in InsertMany?

Generally, No. The usage here would be people looping over their own insert or insert and replace statements and batching them on their own.

Performing some inserts mixed with some insert or replace would be similar to providing mixing deletes and updates in the same batch. Doesn't seem like a fit for the API. IMHO.

At least at this point... because someone will always come up with something that makes sense to themselves :-)

Skye-31 commented 1 year ago

That's a fair point.. Just wondering (this would be my bad from before) is the return type for each method actually correct? I seem to recall inserts returning null 🤔

kingmesal commented 1 year ago

Insert and Insert or Replace will both end up yielding a result object that looks like:

{
  results: null | { changes: 1, lastInsertRowid: 8 },
  duration: 34.923211097717285,
  lastRowId: 51,
  changes: 1,
  success: true,
  served_by: 'x-miniflare.db3'
}

another edit here... your return types seem to match that of the D1 API so there is nothing of concern here... i just ran all the combinations and looks good to me.

Skye-31 commented 1 year ago

We should probably update the types while doing this then. Returning D1Result<null> when the replace option is false, otherwise D1Result<{ changes: number, lastRowID: number }> is probably best? (we could just go for unknown too?)

Currently they're just incorrect which isn't the best. Happy to cover this in my own PR if you prefer though

kingmesal commented 1 year ago

D1Result isn't null in any of these cases.... what I shared above is the output of both the ORM call and D1Database calls:

{
  results?: T[];
  lastRowId: number | null;
  changes: number;
  duration: number;
  error?: string;
};

D1Result.results can be null, by its type def.

Matter of fact, I think the only way D1Result can be null is if an exception occurred and there was no return.

Skye-31 commented 1 year ago

Ah makes sense, will give this a once over in the morning and then release 🙂 Thanks for the contribution!