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

Fix SQLite documentation example. #1750

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

angrytongan
Copy link
Contributor

No description provided.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Is the SQLite page currently broken for you? It works for me. And we would rather use the self-hosted chinook.db than the externally hosted one.

@angrytongan
Copy link
Contributor Author

This is current production https://observablehq.com/framework/lib/sqlite:

Screenshot 2024-10-14 at 05 06 54

rather use the self-hosted chinook.db than the externally hosted one.

Ah, ok. Let me fix up my PR.

@angrytongan angrytongan marked this pull request as draft October 13, 2024 19:08
@mbostock
Copy link
Member

Ah, this must be a Safari bug. I’ll investigate when I’m back at my desktop.

@Fil
Copy link
Contributor

Fil commented Oct 14, 2024

We can reduce this to:

```js echo
import SQLite from "npm:@observablehq/sqlite";
```

```js echo
FileAttachment("chinook.db").sqlite()
```

removing the explicit import SQLite fixes the issue, which seems to confirm that this might be due to https://bugs.webkit.org/show_bug.cgi?id=242740

The issue seems to be caused by the parallel imports of sqlite.js that are made in the two cells.

Here's a different repro which might help investigate:

```js echo
import("npm:@observablehq/sqlite");
```

```js echo
SQLite
```

on Safari, the presence of the first block causes the second code block to be (erroneously) undefined instead of being stdlib’s SQLite object.

@Fil
Copy link
Contributor

Fil commented Oct 14, 2024

To fix the documentation it is enough to set run=false on the cells that explicity load this module:

 [SQLite](https://sqlite.org/) is “a small, fast, self-contained, high-reliability, full-featured, SQL database engine” and “the most used database engine in the world.” Observable provides a ESM-compatible distribution of [sql.js](https://sql.js.org), a WASM-based distribution of SQLite. It is available by default as `SQLite` in Markdown, but you can import it like so:

-```js echo
+```js run=false
 import SQLite from "npm:@observablehq/sqlite";
 ```

 We also provide `SQLiteDatabaseClient`, a [`DatabaseClient`](https://observablehq.com/@observablehq/database-client-specification) implementation.

-```js echo
+```js run=false
 import {SQLiteDatabaseClient} from "npm:@observablehq/sqlite";
 ```

(but that doesn't fix the underlying issue).

@mbostock
Copy link
Member

Wow, so top-level await is broken in Safari? That’ll be hard to avoid. I hope they fix it soon.

@Fil
Copy link
Contributor

Fil commented Oct 14, 2024

👎 Not fixed in Safari Technology Preview (Release 205 (Safari 18.0, WebKit 20621.1.2.111.4))

@mbostock
Copy link
Member

I’ve implemented a workaround for top-level await being broken in Safari in #1759. However, I’m not sure it’s practical for us to workaround such a fundamental browser bug. Several other recommended libraries also use top-level await, include DuckDBClient and dot. It’d be relatively easy to fix DuckDBClient, but I don’t think we can fix dot without making it async.

So… maybe users will need to avoid importing the same library multiple times (until Safari is fixed)?

@mbostock mbostock marked this pull request as ready for review October 14, 2024 22:12
@mbostock mbostock enabled auto-merge (squash) October 14, 2024 22:12
@mbostock mbostock merged commit 21ac188 into observablehq:main Oct 14, 2024
4 checks passed
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.

3 participants