annotate local paths #1731

Fil · 2024-10-08T15:02:31Z

paths to local files in transpiled JavaScript modules are postfixed with a comment

For example, when building the documentation, we now have in docs/.observablehq/dist/_import/chart.95e875cf.js

path: "../_file/lib/gistemp.08e51068.csv"/* observablehq-file */,

in lieu of

path: "../_file/lib/gistemp.08e51068.csv",

The annotate.js scripts lists a few cases that can be annotated.

mbostock · 2024-10-08T16:39:51Z

src/javascript/transpile.ts

+ * Annotate a path to a local import or file so it can be reworked server-side.
+ */
+function annotatePath(uri: string): string {
+  return !uri.startsWith(".") || /(?:[.][/]|(?:[.][.][/])+)(_jsr|_node|_npm|_observablehq)[/]/.test(uri)


This should be isAssetPath or whatever that’s ['./', '../', '/']. Don’t want to match against .foo.

mythmon · 2024-10-15T23:17:40Z

Sorry for the delay. I plan to review this tomorrow.

src/javascript/transpile.ts

mythmon · 2024-10-17T22:31:02Z

test/javascript/transpile-test.ts

@@ -210,11 +210,11 @@ describe("transpileModule(input, root, path)", () => {
  it("rewrites npm imports", async () => {
    const input = 'import "npm:d3-array";';
    const output = (await transpileModule(input, options)).split("\n").pop()!;
-    assert.strictEqual(output, 'import "../_npm/[email protected]/_esm.js";');
+    assert.strictEqual(output, 'import "../_npm/[email protected]/_esm.js"/* observablehq-file */;');


If this link is tagged as observable-file, then we should also rewrite it to tag Observable files in it. If we don't rewrite it, I don't think we should tag it.

I was initially doing that (see line 307 in 6cfb74d#diff-a479e6903ca67c840829983998e33849155911842cf08fa118b42b2d88fe3c9b), then I think we decided to tag everything and let the server decide what it should do with it.

My concern is that this is a reference to an _npm file that is tagged, but references to _npm files from other _npm files are not tagged. As a concrete example, in the docs build there is this chain:

dist/chart.js tags its import to the _import file:

docs/.observablehq/dist/_import/chart.95e875cf.js tags its import of an _npm file:

docs/.observablehq/dist/_npm/@observablehq/[email protected]/e828d8c8.js does not tag its import of the _npm file:

../../[email protected]/c68fbd73.js

It would be helpful if the tagging was done based on the taget of the URL, not the place the URL occurs. As it stands, the server can't really make the decision fully, because links to _npm files aren't consistent tagged/not tagged, so it has to special case them as not needing extra authentication.

test/output/build/data-loaders/_import/test.86a60bc6.js

mythmon

I think this will work. Thanks!

mythmon · 2024-10-25T18:25:36Z

@Fil is there anything more you want to do on this PR before merging it? It would be nice to have this in Framework's main branch for further development.

Fil · 2024-10-25T18:27:12Z

Not on my side; I just wanted to give Mike a chance to review.

mbostock

Do we want this to be always on, or do we only want to turn it on when building for Cloud? I’d slightly prefer the latter and believe that was what we proposed (via environment variable)?

Fil · 2024-10-28T10:22:31Z

Annotations are now conditioned on the ANNOTATE_FILES=true environment variable. (Any other value throws an error.)

mootari · 2024-10-28T11:10:48Z

We should probably make that OBSERVABLE_ANNOTATE_FILES?

Fil · 2024-10-28T12:53:19Z

Sure, or OBSERVABLE_ANNOTATE for short. I don't have an opinion on this :)

mythmon · 2024-10-28T17:38:14Z

I'd prefer if all environment variables that need to be set in Cloud Builds begin with OBSERVABLE_. I don't have a preference for OBSERVABLE_ANNOTATE vs OBSERVABLE_ANNOTATE_FILES.

mbostock

Noting a few things I plan on fixing.

mbostock · 2024-11-01T21:31:21Z

package.json

-    "test:mocha:serial": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 30000 \"test/build/test/**/*-test.js\"",
+    "test:mocha": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 30000 -p \"test/build/test/**/*-test.js\" && yarn test:annotate",
+    "test:mocha:serial": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 30000 \"test/build/test/**/*-test.js\" && yarn test:annotate",
+    "test:annotate": "yarn test:build && cross-env OBSERVABLE_ANNOTATE_FILES=true TZ=America/Los_Angeles mocha --timeout 30000 \"test/build/test/**/annotate.js\"",


This pattern breaks yarn test:mocha:serial with it.only or describe.only because it always runs the test:annotate in addition to your only’d tests.

mbostock · 2024-11-01T21:31:57Z

src/javascript/annotate.ts

+const annotate = process.env["OBSERVABLE_ANNOTATE_FILES"];
+if (typeof annotate === "string" && annotate !== "true")
+  throw new Error(`unsupported OBSERVABLE_ANNOTATE_FILES value: ${annotate}`);
+export default annotate


We don’t use default exports (because there’s no real reason to, and it encourages more inconsistency in how people name an imported symbol). This should be called annotatePath or something.

Fil added 6 commits October 8, 2024 16:48

annotate local paths (_import and _file)

6cfb74d

test

06b5fee

transpile

5b7ad8d

more transpile

1136735

transpiled modules only

4224b3a

dynamicPath annotate.js

d706f6c

mbostock reviewed Oct 8, 2024

View reviewed changes

Fil added 5 commits October 9, 2024 18:29

this part is about pages, do not annotate

498d37b

flatten file attachments json

b69ad62

isPathImport

aec5051

tests

829e087

remove early test

fabeb4f

Fil requested a review from mythmon October 10, 2024 16:48

Fil marked this pull request as ready for review October 10, 2024 16:48

mythmon requested changes Oct 17, 2024

View reviewed changes

Fil added 3 commits October 18, 2024 06:59

Merge branch 'main' into fil/annotate

963aeb8

fix tests

7c96c68

Annotate scripts in _node, _npm, and _jsr

e0bc373

Fil force-pushed the fil/annotate branch from c827ba3 to e0bc373 Compare October 21, 2024 19:34

mythmon approved these changes Oct 21, 2024

View reviewed changes

mbostock reviewed Oct 25, 2024

View reviewed changes

Fil added 3 commits October 28, 2024 10:34

Merge branch 'main' into fil/annotate

568a4bc

annotate only if env.ANNOTATE_FILES

c81145e

test annotations

1001dde

OBSERVABLE_ANNOTATE_FILES

49acd88

Fil enabled auto-merge (squash) October 28, 2024 20:29

Fil merged commit b66e152 into main Oct 28, 2024
4 checks passed

Fil deleted the fil/annotate branch October 28, 2024 20:31

mbostock reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

annotate local paths #1731

annotate local paths #1731

Fil commented Oct 8, 2024 •

edited

Loading

mbostock Oct 8, 2024

mythmon commented Oct 15, 2024

mythmon Oct 17, 2024

Fil Oct 18, 2024

mythmon Oct 18, 2024

mythmon left a comment

mythmon commented Oct 25, 2024

Fil commented Oct 25, 2024

mbostock left a comment

Fil commented Oct 28, 2024

mootari commented Oct 28, 2024

Fil commented Oct 28, 2024

mythmon commented Oct 28, 2024

mbostock left a comment

mbostock Nov 1, 2024

mbostock Nov 1, 2024

annotate local paths #1731

annotate local paths #1731

Conversation

Fil commented Oct 8, 2024 • edited Loading

mbostock Oct 8, 2024

Choose a reason for hiding this comment

mythmon commented Oct 15, 2024

mythmon Oct 17, 2024

Choose a reason for hiding this comment

Fil Oct 18, 2024

Choose a reason for hiding this comment

mythmon Oct 18, 2024

Choose a reason for hiding this comment

mythmon left a comment

Choose a reason for hiding this comment

mythmon commented Oct 25, 2024

Fil commented Oct 25, 2024

mbostock left a comment

Choose a reason for hiding this comment

Fil commented Oct 28, 2024

mootari commented Oct 28, 2024

Fil commented Oct 28, 2024

mythmon commented Oct 28, 2024

mbostock left a comment

Choose a reason for hiding this comment

mbostock Nov 1, 2024

Choose a reason for hiding this comment

mbostock Nov 1, 2024

Choose a reason for hiding this comment

Fil commented Oct 8, 2024 •

edited

Loading