-
Notifications
You must be signed in to change notification settings - Fork 250
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(quaint): enable wasm32-unknown-unknown
compilation via slim
feature flag
#4119
Conversation
Signed-off-by: Alex Chi <[email protected]>
Signed-off-by: Alex Chi <[email protected]>
Signed-off-by: Alex Chi <[email protected]>
Signed-off-by: Alex Chi <[email protected]>
Signed-off-by: Alex Chi <[email protected]>
…gines into jkomyno/quaint-wasm
CodSpeed Performance ReportMerging #4119 will not alter performanceComparing Summary
|
wasm32-unknown-unknown
compilation via slim
feature flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we revert the changes to Cargo.lock? It doesn't look like they add much, just noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One preliminary thought, is that if we don't regression test this in CI, we're bound to break wasm32-unknown-unknown compilation with a random PR or cargo update
.
Hi @skyzh — this is awesome work and it is definitely going to be useful when we tackle Wasm, hopefully in the near future. Since we would be merging this code without exercising it, that would amount to merging additional complexity in the form of conditional compilation because it may be useful later, so a few of us discussed yesterday and decided to hold off on merging for now. We may want to reduce the amounts of feature flags in quaint, and separate the database drivers part of quaint from the pure logic before we tackle this. |
Thanks for letting me know! Feel free to ping me if there is anything I can help :) |
Superseded by #4442. |
This PR enables compiling
quaint
towasm32-unknown-unknown
when using the feature flagslim
:Context
There is a related PR (#4064) that extracts quaint to a separate quaint-core crate. I had been working on a patch over that but there are several issues with that PR:
QuaintValue
, it will need to be defined inquaint-core
, but traits liketokio_postgres::ToSql
will be implemented inquaint
, which is not allowed by Rust because bothToSql
andQuaintValue
are not part of the crate that implements the trait. We will need to implement a wrapper and change all usages ofQuaintValue
to the wrapper if we proceed with the extracting quaint-core approach.quaint-core
/quaint
.I spent 1.5 days working upon #4064 without getting it compiled and therefore I decided to continue with the current approach, which only takes 2 hours to finish the refactor.
In this PR, we mainly refactored the
connector
part andsingle.rs
of thequaint
crate to make it compile in WASM.postgresql-connector
,mysql-connector
,mysql-connector
, andsqlite-connector
, which are the switches to import the actual database connector implementation into the quaint crate.slim
feature that is exclusively used by the WASM compilation. Compiling with this feature does not include the connectors but all the code for parsing database URL, transforming SQLs, etc.xxx_common.rs
andxxx.rs
. All code are copy-pasted with only tiny changes like changing the import qualifier (i.e.,tokio_postgres::config::SslMode
instead ofSslMode
). All other things remain unchanged.Compiling quaint into WASM would be the milestone that enables Prisma running in edge runtime. I think we are 50% done here, and the remaining work is to get other crates on the path to compile and add a new wasm driver.
This is a big refactor that might conflict with ongoing PRs on the quaint crate. Feel free to ping me for rebasing / resolving merge conflicts. Thanks for the reviews in advance!
This PR closes https://github.com/prisma/team-orm/issues/279.
The original PR (with conflicts) is at: #4095.