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

detect non-local sql tables #1636

Merged
merged 2 commits into from
Sep 3, 2024
Merged

detect non-local sql tables #1636

merged 2 commits into from
Sep 3, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Sep 3, 2024

Fixes #1635.

@mbostock mbostock requested a review from Fil September 3, 2024 14:26
@mbostock mbostock enabled auto-merge (squash) September 3, 2024 14:27
@Fil
Copy link
Contributor

Fil commented Sep 3, 2024

We can probably refine a little with this change:

diff --git a/src/render.ts b/src/render.ts
index 926f24e1..4b2f2efe 100644
--- a/src/render.ts
+++ b/src/render.ts
@@ -59,7 +59,7 @@ if (location.pathname.endsWith("/")) {
 import ${preview || page.code.length ? `{${preview ? "open, " : ""}define} from ` : ""}${JSON.stringify(
     resolveImport("observablehq:client")
   )};${
-    files.size || data?.sql
+    files.size
       ? `\nimport {registerFile${data?.sql ? ", FileAttachment" : ""}} from ${JSON.stringify(
           resolveImport("observablehq:stdlib")
         )};`

and skip an unnecessary import of stdlib. It passes all the tests but at this point I'm wary of introducing a last-minute bug.

@mbostock
Copy link
Member Author

mbostock commented Sep 3, 2024

Okay, added. I tested, but I can’t load the Parquet file from blobs.duckdb.org anymore because of a CORS error, which is odd because I thought it worked before. The generated code looks fine though. 🤷

@Fil
Copy link
Contributor

Fil commented Sep 3, 2024

It worked before for me too, and now I also got the CORS error. But substituting a different URL confirms this works.

(We still have <link rel="modulepreload" href="./_observablehq/stdlib.c0fee887.js">, so maybe it isn't much better.)

@mbostock mbostock merged commit fa80489 into main Sep 3, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/fix-sql-assets branch September 3, 2024 15:29
@mbostock
Copy link
Member Author

mbostock commented Sep 3, 2024

I don’t think it’s possible to avoid loading the Observable standard library entirely; see src/client/main.js. Still fine to avoid the unnecessary import though and save a few bytes. 😁

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.

framework tries to build a remote file listed in sql front-matter
2 participants