-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(db): add support for non-node libsql client #14204
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 5604a6f The changes in this PR will be included in the next version bump. 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 |
CC @Fryuni does this look correct? |
packages/db/src/runtime/db-client.ts
Outdated
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.
In theory @libsql/client
automatically switches to the appropriate implementation for the runtime it is running on. So if the missing module is coming from LibSQL this is a bug there.
If it is coming from Drizzle Client's module then I don't know if it was supposed to work or not. Worth bringing it up on their discord or repo.
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.

My understanding would be that as well... but once i looked at how libsql client "decides" which one to use.... I think i found why it doesn't work... since when your deploying to CF, its still running node underlying when doing the import.
As for drizzle docs... they have this note
defaults to
node
import, automatically changes toweb
iftarget
orplatform
is set for bundler, e.g.esbuild --platform=browser
packages/db/src/runtime/db-client.ts
Outdated
import type { LibSQLDatabase } from 'drizzle-orm/libsql'; | ||
import { drizzle as drizzleLibsql } from 'drizzle-orm/libsql'; | ||
import { createClient as createClientWeb } from '@libsql/client/web'; | ||
import { drizzle as drizzleLibsql, type LibSQLDatabase } from 'drizzle-orm/libsql'; |
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.
Won't Cloudflare complain just by importing this module? If that is the case, we'll need to do some tricks using virtual modules to only import the modules that are safe for the runtime.
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.
Yeah i was wondering the same thing... I'll convert the db client config parts into dedicated internal virtual modules that will be enabled depending on mode instead, here after i make my coffee.
fix(db): ensure local client module is returned in default case
OKAY... @Fryuni I think its at the point that everything is working (finally fixed the tests too), and I've virtualized all the libsql/drizzle clients... that was a bit annoying to do 😅 but should be worth it! I think its ready to be moved from draft? |
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.
Looking really good
packages/db/src/runtime/utils.ts
Outdated
export function parseOpts(config: Record<string, string>): Partial<LibSQLConfig> { | ||
return { | ||
...config, | ||
...(config.syncInterval ? { syncInterval: parseInt(config.syncInterval) } : {}), | ||
...('readYourWrites' in config ? { readYourWrites: config.readYourWrites !== 'false' } : {}), | ||
...('offline' in config ? { offline: config.offline !== 'false' } : {}), | ||
...('tls' in config ? { tls: config.tls !== 'false' } : {}), | ||
...(config.concurrency ? { concurrency: parseInt(config.concurrency) } : {}), | ||
}; |
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't we use Zod for this? It has coercion and transformations.
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.
........ now that you say that.... yes... yes we could... why i didnt do that in the first place? no freaking clue...
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.
okay got the zod transformer built and working 😄
}: { | ||
tables: DBTables; | ||
appToken: string; | ||
isBuild: boolean; | ||
output: AstroConfig['output']; | ||
__execute?: boolean; |
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.
__execute?: boolean; | |
// Request module to be loaded immediately in process | |
localExecution?: boolean; |
case 'web': { | ||
return `export { createClient } from '${DB_CLIENTS.web}';`; | ||
} |
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.
case 'web': { | |
return `export { createClient } from '${DB_CLIENTS.web}';`; | |
} | |
case 'web': | |
return `export { createClient } from '${DB_CLIENTS.web}';`; |
…onfiguration handling
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.
Woo! What an accomplishment! Quick thoughts from me...
(Also noting, I don't see any new error messages, so checking that we don't need any new specific ones, nor an update to any existing because our existing ones don't know about this option)
Thank god for the save by the docs queen Co-authored-by: Sarah Rainsberger <[email protected]>
Only user facing change is the new options added to DB, that previously never existed (not even an object inside the integration function) everything else is the same "internals" just now with the ability to pick web or node drivers (since for some reason the auto detection by the libsql/client doesn't work... likely related to how Astro's build pipeline works under the hood) |
export function getLocalVirtualModContents({ | ||
tables, | ||
root, | ||
localExecution = false, | ||
}: { |
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 you add some JSDocs that explain the business logic of the function? The new localExecution
adds some logic that isn't clear to me. For example, it doesn't explain why, when localExecution
is true
, we need the node client. It's an assumption that isn't explained.
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.
The node client has full protocol support, file:
, libsql:
, http:
. and https:
where as the web client is missing the file:
support (for obvious reasons) So for users who utilize the file:
protocol (when we are running the execute
command) we need to ensure that we have the node client available.
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.
I've extracted the duplicated logic to a helper function, and added jsdocs to the new function and the two instances of localExecution
}: { | ||
tables: DBTables; | ||
appToken: string; | ||
isBuild: boolean; | ||
output: AstroConfig['output']; | ||
// Request module to be loaded immediately in process | ||
localExecution?: boolean; |
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.
If possible, I would avoid the ?
here. Why? Because it's an internal code, and by adding ?
we might miss code paths that might require true
instead. This is usually a pattern I follow inside code that isn't exposed to the user: the compiler fails all the instances of the code, and forces me to update them all. Would you mind doing that here and in the other function?
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.
Updated in latest commit
const clientImport = localExecution | ||
? `import { createClient } from '${DB_CLIENTS.node}';` | ||
: `import { createClient } from '${VIRTUAL_CLIENT_MODULE_ID}';`; |
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.
To me, using a node client for a remote feels like an error. However, I am sure you think it's not. Is it because the execution of commands can be done only locally? If so, then I have two suggestions:
- commands shouldn't call
getRemoteVirtualModContents
- add some JsDoc to
getRemoteVirtualModContents
that explains the implications oflocalExecution
for this function
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.
see #14204 (comment) for the context, we previously only had the node client, and that is the recommended approach by drizzle and libsql teams unless you specifically need one of the other limited exports. (as a file:
protocol CAN be considered "remote")
Co-authored-by: Emanuele Stoppa <[email protected]>
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.
Thank you for addressing my questions!
So @sarah11918 I'm guessing that the best place for the new config options docs wise would be replacing the note under this section? |
@Adammatthiesen That note can certainly be removed! I will note that section is about connecting to remote databases, and so maybe we don't want to clutter inside one set of instructions (connecting) with an entirely new topic (a configuration option). I don't think the Then, in the "connect to a remote database" instructions, if something is different for |
The difference is the fact that the I also agree that we will need the full config reference as well, most likely just adding to the dedicated integration page? https://docs.astro.build/en/guides/integrations-guide/db/ |
I'm confused, the first link you posted was the db integration guide? that's the only guide in question here, right? |
there is two pages... the guide page, and the technical page.... so maybe both... thats why im asking you after all... your the docs person who knows best. 😅 |
Haha, sorry, I thought you were referring to the same page twice. I'll figure out this docs thing eventually... |
OK, so catching up on this yes, we do need this config option mentioned on the integration page itself. Typically, a Then yes, in "connecting to remote databases" replace the note with a sub-heading, something maybe like Does that sound about right to you? |
Probably use the term "driver" instead of client? but yup sounds perfect. I'll work on the docs here in the next few days! |
}); | ||
``` | ||
|
||
For more information, see the [`@astrojs/db` documentation](https://docs.astro.build/en/guides/integrations-guide/db/). |
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.
For more information, see the [`@astrojs/db` documentation](https://docs.astro.build/en/guides/integrations-guide/db/). | |
For more information, see the [`@astrojs/db` documentation](https://docs.astro.build/en/guides/integrations-guide/db/#mode). |
We can come back and update this with the direct link to the mode
configuration option after the docs are written! I suspect it will end up being just #mode
.
Changes
This PR solves the outstanding issue that was being caused by the usage of the
@libsql/client
's default export of their node client, by introducing a new option to switch from the node client to the web client. Allowing users on platforms like Cloudflare to finally be able to use Astro DB since the Studio sunset.Testing
There is currently no tests testing external server connections. And im not sure how possible it would be to spin up a libsql server locally in CI for tests.
Docs
CC @sarah11918 at your request i'm tagging you!