Skip to content
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

added insert or replace #41

Merged

Conversation

kingmesal
Copy link
Contributor

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

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2022

🦋 Changeset detected

Latest commit: 9049500

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
Copy link
Contributor

Skye-31 commented Sep 27, 2022

Thanks for the PR!
3 things:

  • Could you add a changeset please? npx changeset in your terminal, -> semver Minor
  • Do we want to allow people to set it per item in InsertMany? Could perhaps refactor the options into { data: Partial<T>, orReplace?: boolean }[]. This is okay to do as v0.x has looser semver requirements, just add a note in the changeset if you decide to do this
  • Also looks like prettier is causing the checks to fail (npm run prettify will resolve it)

@kingmesal
Copy link
Contributor Author

kingmesal commented Sep 27, 2022

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
Copy link
Contributor

Skye-31 commented Sep 27, 2022

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
Copy link
Contributor Author

kingmesal commented Sep 27, 2022

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
Copy link
Contributor

Skye-31 commented Sep 27, 2022

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
Copy link
Contributor Author

kingmesal commented Sep 27, 2022

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
Copy link
Contributor

Skye-31 commented Sep 27, 2022

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

@Skye-31 Skye-31 merged commit f8ca119 into Interactions-as-a-Service:main Sep 28, 2022
@github-actions github-actions bot mentioned this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants