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

feat(driver-adapters): add "version()" support to QuaintQueryable #5103

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Jan 4, 2025

This PR adds a version() implementation to QuaintQueryable in query-engine/driver-adapters/src/queryable.rs.
This is needed for schema-engine-wasm.

@jkomyno jkomyno requested a review from a team as a code owner January 4, 2025 14:39
@jkomyno jkomyno requested review from jacek-prisma and removed request for a team January 4, 2025 14:39
@jkomyno jkomyno added this to the 6.2.0 milestone Jan 4, 2025
Copy link

codspeed-hq bot commented Jan 4, 2025

CodSpeed Performance Report

Merging #5103 will not alter performance

Comparing feat/add-version-to-js-queryable (3d71c33) with main (c49e56c)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Jan 6, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.096MiB 2.095MiB 1.071KiB
Postgres (gzip) 841.366KiB 840.894KiB 483.000B
Mysql 2.057MiB 2.056MiB 1.071KiB
Mysql (gzip) 827.872KiB 827.513KiB 367.000B
Sqlite 1.972MiB 1.971MiB 1.086KiB
Sqlite (gzip) 792.451KiB 792.150KiB 308.000B

@@ -130,9 +130,25 @@ impl QuaintQueryable for JsBaseQueryable {
.await
}

// Note: Needed by the Wasm Schema Engine only.
async fn version(&self) -> quaint::Result<Option<String>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this call might be covered in some existing schema engine test, but I think it might be worth writing a basic explicit test case to ensure it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't any infra to run Driver Adapters in Schema Engine tests right now, so this cannot be achieved currently. But in principle, I'd agree with you :)

Copy link
Contributor

@jacek-prisma jacek-prisma left a comment

Choose a reason for hiding this comment

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

1 minor comment but lgtm otherwise

@jkomyno jkomyno merged commit 4123509 into main Jan 7, 2025
368 checks passed
@jkomyno jkomyno deleted the feat/add-version-to-js-queryable branch January 7, 2025 10:43
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