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(query-engine-common): reduce duplicated code between query-engine-node-api and query-engine-wasm #4521

Merged
merged 192 commits into from Dec 18, 2023

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Dec 5, 2023

This PR closes https://github.com/prisma/team-orm/issues/692.

This PR is a follow-up of #4466.

/integration

jkomyno and others added 30 commits November 10, 2023 13:49
…github.com:prisma/prisma-engines into feat/sql-query-connector-on-wasm32-unknown-unknown
* qe-wasm: Partially fix the test suite

1. Bash script for build did not abort on error, hence sed error on
   linux went unnoticed.
2. We don't actually need sed trickery sincce `wasm-pack` has a flag for
   changing binary name.
3. `tracing` feature does not actually work on WASM even partially: it panics on
   `Instant` invokation as soon as first span is created. Since we are
   running tests with all preview features enabled, that means that
   practically any test panics now. Disabled it again.

A lot of tests are still failing on ThreadRng invocation and stacktrace
is not really helpful, but I still think it's better if we get it
working.

* Update query-engine/query-engine-wasm/build.sh
@SevInf SevInf force-pushed the feat/query-engine-wasm32-unknown-unknown branch 2 times, most recently from d7ad5d5 to c7c8586 Compare December 6, 2023 10:25
@jkomyno jkomyno marked this pull request as ready for review December 6, 2023 14:08
@jkomyno jkomyno requested a review from a team as a code owner December 6, 2023 14:08
@jkomyno jkomyno requested review from miguelff and Weakky and removed request for a team December 6, 2023 14:08
Copy link
Member

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of that! Are there reasons why 👇🏻 cannot be part of query-engine-common too?

/// The state of the engine.
enum Inner {
/// Not connected, holding all data to form a connection.
Builder(EngineBuilder),
/// A connected engine, holding all data to disconnect and form a new
/// connection. Allows querying when on this state.
Connected(ConnectedEngine),
}
/// Everything needed to connect to the database and have the core running.
struct EngineBuilder {
schema: Arc<psl::ValidatedSchema>,
config_dir: PathBuf,
env: HashMap<String, String>,
engine_protocol: EngineProtocol,
}
/// Internal structure for querying and reconnecting with the engine.
struct ConnectedEngine {
schema: Arc<psl::ValidatedSchema>,
query_schema: Arc<QuerySchema>,
executor: crate::Executor,
config_dir: PathBuf,
env: HashMap<String, String>,
metrics: Option<MetricRegistry>,
engine_protocol: EngineProtocol,
}
/// Returned from the `serverInfo` method in javascript.
#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
struct ServerInfo {
commit: String,
version: String,
primary_connector: Option<String>,
}
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct MetricOptions {
format: MetricFormat,
#[serde(default)]
global_labels: HashMap<String, String>,
}

@medz
Copy link

medz commented Dec 7, 2023

Thumbs up! I've been waiting for it for a year! Finally, third-party clients no longer have to copy Prisma engines code! Engine upgrades will not bring devastating damage to third-party clients!

@medz
Copy link

medz commented Dec 7, 2023

Could more be done? For example, it is the same as described in #3757, because a large amount of logic code is common between wasm engine and napi engine.

Base automatically changed from feat/query-engine-wasm32-unknown-unknown to main December 8, 2023 09:03
@jkomyno
Copy link
Contributor Author

jkomyno commented Dec 11, 2023

Thanks for taking care of that! Are there reasons why 👇🏻 cannot be part of query-engine-common too?

/// The state of the engine.
enum Inner {
/// Not connected, holding all data to form a connection.
Builder(EngineBuilder),
/// A connected engine, holding all data to disconnect and form a new
/// connection. Allows querying when on this state.
Connected(ConnectedEngine),
}
/// Everything needed to connect to the database and have the core running.
struct EngineBuilder {
schema: Arc<psl::ValidatedSchema>,
config_dir: PathBuf,
env: HashMap<String, String>,
engine_protocol: EngineProtocol,
}
/// Internal structure for querying and reconnecting with the engine.
struct ConnectedEngine {
schema: Arc<psl::ValidatedSchema>,
query_schema: Arc<QuerySchema>,
executor: crate::Executor,
config_dir: PathBuf,
env: HashMap<String, String>,
metrics: Option<MetricRegistry>,
engine_protocol: EngineProtocol,
}
/// Returned from the `serverInfo` method in javascript.
#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
struct ServerInfo {
commit: String,
version: String,
primary_connector: Option<String>,
}
#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
struct MetricOptions {
format: MetricFormat,
#[serde(default)]
global_labels: HashMap<String, String>,
}

Good question! There is no good reason to exclude that, besides that it'd need some additional conditional checks.
I've implemented your suggestion in 1af7189, let me know what you think ☺️

@jkomyno
Copy link
Contributor Author

jkomyno commented Dec 11, 2023

Oh, and ServerInfo is also shared between query-engine-node-api and query-engine-wasm, but that's probably safe to remove (I'll continue #4524 for "tech debt Friday").

Copy link
Contributor

github-actions bot commented Dec 15, 2023

WASM Size

Engine This PR Base branch Diff
WASM 3.185MiB 3.184MiB 1.187KiB
WASM (gzip) 1.200MiB 1.199MiB 741.000B

@jkomyno jkomyno merged commit ae99f51 into main Dec 18, 2023
64 checks passed
@jkomyno jkomyno deleted the feat/query-engine-common branch December 18, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants