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

avoid top-level await; import npm:sql.js #1759

Merged
merged 6 commits into from
Nov 2, 2024
Merged

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 14, 2024

Ref. #1750. Avoids top-level await in our SQLite implementation. Also makes it so that importing npm:sql.js “just works” (by rewriting the code slightly from what is distributed via npm!). Removes SQLite from standard library (because there’s no way to support it without using top-level await) in favor of having people import sql.js directly or use SQLiteDatabaseClient.

Marking as draft because the hacky rewriting of sql.js should be cleaned up before merging.

Also we might also want to fix DuckDBClient? And dot? But I don’t think dot is fixable without making it async…

@mbostock mbostock requested a review from Fil October 14, 2024 22:00
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

Any blocker left? It has a small merge conflict with #1734 but easy to solve (and tested 👍 )

@mbostock
Copy link
Member Author

Just want to clean up the sql.js shim if I can.

@mbostock
Copy link
Member Author

The other thing I’m ruminating on is that this isn’t strictly a backwards-compatible change. Previously this would be equivalent to the SQLite that is available by default in Markdown:

import SQLite from "npm:@observablehq/sqlite"; 

Now, however, the explicit import gives you a promise. That’s kinda the same as long as you reference SQLite from another fenced code block. But technically it’s not backwards-compatible. And it would be more noticeable in a local JavaScript module (where you don’t get implicit await).

So… We’re kinda stuck with top-level await in the SQLite case if we want to maintain backwards compatibility. But maybe it’s okay to change the semantics slightly, especially if it’s rare and given that top-level await is broken in Safari? Maybe I should break out the DuckDB-Wasm changes since those don’t have the same concern.

@Fil
Copy link
Contributor

Fil commented Oct 21, 2024

OK… I wouldn't worry too much about the slight change in API because… the default SQLite symbol is not documented in https://observablehq.com/framework/lib/sqlite (I mean, it's referenced, but nothing says how to use it); only SQLiteDatabaseClient is shown. We also need to fix the documentation :)

@mbostock mbostock marked this pull request as ready for review November 2, 2024 16:52
@Fil
Copy link
Contributor

Fil commented Nov 2, 2024

lgtm

@mbostock mbostock merged commit 2b4a42b into main Nov 2, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/fix-sqlite branch November 2, 2024 19:28
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