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

fix(driver-adapters): PlanetScale transactions #4967

Merged
merged 36 commits into from
Sep 5, 2024

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jul 24, 2024

This PR deprecates #4966.

This PR:

Companion TypeScript PR: prisma/prisma#24878.
Please refer to the explanations therein about TransactionContext.

@jkomyno jkomyno requested a review from a team as a code owner July 24, 2024 16:01
@jkomyno jkomyno requested review from laplab and removed request for a team July 24, 2024 16:01
@jkomyno jkomyno self-assigned this Jul 24, 2024
Copy link

codspeed-hq bot commented Jul 24, 2024

CodSpeed Performance Report

Merging #4967 will not alter performance

Comparing integration/fix-planetscale-transactions (0deadde) with main (f2561ec)

Summary

✅ 11 untouched benchmarks

@jkomyno jkomyno requested review from Weakky and removed request for laplab August 12, 2024 21:00
Copy link
Contributor

github-actions bot commented Aug 12, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.064MiB 2.062MiB 1.988KiB
Postgres (gzip) 824.513KiB 823.313KiB 1.201KiB
Mysql 2.034MiB 2.032MiB 2.117KiB
Mysql (gzip) 811.424KiB 811.001KiB 433.000B
Sqlite 1.925MiB 1.924MiB 1.952KiB
Sqlite (gzip) 769.174KiB 768.301KiB 894.000B

@jkomyno jkomyno added this to the 5.19.0 milestone Aug 12, 2024
@jkomyno jkomyno requested review from SevInf and removed request for Weakky August 26, 2024 10:08
@jkomyno jkomyno modified the milestones: 5.19.0, 5.20.0 Aug 26, 2024
}

#[async_trait]
impl Queryable for JsTransactionContext {
Copy link
Member

@aqrln aqrln Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of a slippery slope but I really feel like implementing Deref is appropriate in these cases. Let's not do it in this PR to avoid churning unrelated code, but I'd strongly consider doing it later. Others may disagree though.

Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I was sceptical at fist, after thinking about it a bit more I agree that it's an appropriate abstraction. We still have a kind of typestate pattern (i.e. TransactionContext doesn't have methods like commit/rollback, you need to call start_transaction to get access to them) while the Queryable trait represents another dimension and is orthogonal to those methods.

While ideally I would still prefer to have tighter interfaces or push more logic to the driver adapter without going back and forth between JS and Rust so much, that would be a big refactoring, while this PR is indeed a good solution based on the abstractions we have.

@jkomyno jkomyno merged commit 190a98f into main Sep 5, 2024
362 checks passed
@jkomyno jkomyno deleted the integration/fix-planetscale-transactions branch September 5, 2024 11:32
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.

2 participants