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.

Sign up for GitHub

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

Jump to bottom

annotate local paths #1731

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

annotate local paths #1731

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6cfb74d
annotate local paths (_import and _file)
Fil Oct 8, 2024
06b5fee
test
Fil Oct 8, 2024
5b7ad8d
transpile
Fil Oct 8, 2024
1136735
more transpile
Fil Oct 8, 2024
4224b3a
transpiled modules only
Fil Oct 8, 2024
d706f6c
dynamicPath annotate.js
Fil Oct 8, 2024
498d37b
this part is about pages, do not annotate
Fil Oct 9, 2024
b69ad62
flatten file attachments json
Fil Oct 9, 2024
aec5051
isPathImport
Fil Oct 9, 2024
829e087
tests
Fil Oct 10, 2024
fabeb4f
remove early test
Fil Oct 10, 2024
963aeb8
Merge branch 'main' into fil/annotate
Fil Oct 18, 2024
7c96c68
fix tests
Fil Oct 18, 2024
e0bc373
Annotate scripts in _node, _npm, and _jsr
Fil Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions src/javascript/transpile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export async function transpileModule(

async function rewriteImportSource(source: StringLiteral) {
const specifier = getStringLiteralValue(source);
output.replaceLeft(source.start, source.end, JSON.stringify(await resolveImport(specifier)));
output.replaceLeft(source.start, source.end, annotatePath(await resolveImport(specifier)));
}

for (const {name, node} of findFiles(body, path, input)) {
Expand All @@ -111,17 +111,15 @@ export async function transpileModule(
output.replaceLeft(
source.start,
source.end,
`${JSON.stringify(
`${
info
? {
name: p,
mimeType: mime.getType(name) ?? undefined,
path: relativePath(servePath, resolveFile(name)),
lastModified: info.mtimeMs,
size: info.size
}
: p
)}, import.meta.url`
? `{"name":${JSON.stringify(p)},"mimeType":${JSON.stringify(
mime.getType(name) ?? undefined
)},"path":${annotatePath(relativePath(servePath, resolveFile(name)))},"lastModified":${JSON.stringify(
info.mtimeMs
)},"size":${JSON.stringify(info.size)}`
Fil marked this conversation as resolved.
Show resolved Hide resolved
: JSON.stringify(p)
}, import.meta.url`
);
}

Expand All @@ -137,7 +135,7 @@ export async function transpileModule(
if (isImportMetaResolve(node) && isStringLiteral(source)) {
const value = getStringLiteralValue(source);
const resolution = isPathImport(value) && !isJavaScript(value) ? resolveFile(value) : await resolveImport(value);
output.replaceLeft(source.start, source.end, JSON.stringify(resolution));
output.replaceLeft(source.start, source.end, annotatePath(resolution));
}
}

Expand Down Expand Up @@ -259,3 +257,10 @@ export function rewriteParams(output: Sourcemap, body: Node, params: Params, inp
output.replaceLeft(node.start, node.end, JSON.stringify(params[name]));
}
}

/**
* Annotate a path to a local import or file so it can be reworked server-side.
*/
function annotatePath(uri: string): string {
return `${JSON.stringify(uri)}${isPathImport(uri) ? "/* observablehq-file */" : ""}`;
}
4 changes: 2 additions & 2 deletions test/javascript/transpile-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */;');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. dist/chart.js tags its import to the _import file:
  2. docs/.observablehq/dist/_import/chart.95e875cf.js tags its import of an _npm file:
  3. docs/.observablehq/dist/_npm/@observablehq/[email protected]/e828d8c8.js does not tag its import of the _npm file:
  4. ../../[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.

});
it("rewrites node imports", async () => {
const input = 'import "d3-array";';
const output = (await transpileModule(input, options)).split("\n").pop()!;
assert.strictEqual(output, 'import "../_node/[email protected]/index.js";');
assert.strictEqual(output, 'import "../_node/[email protected]/index.js"/* observablehq-file */;');
});
});
2 changes: 1 addition & 1 deletion test/output/build/data-loaders/_import/import-test.e7269c4e.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export {test} from "./test.86a60bc6.js";
export {test} from "./test.86a60bc6.js"/* observablehq-file */;
4 changes: 2 additions & 2 deletions test/output/build/data-loaders/_import/test.86a60bc6.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import {FileAttachment} from "../_observablehq/stdlib.00000003.js";
import {FileAttachment} from "../_observablehq/stdlib.00000003.js"/* observablehq-file */;

export const test = FileAttachment({"name":"../test.txt","mimeType":"text/plain","path":"../_file/test.f2ca1bb6.txt","lastModified":/* ts */1706742000000,"size":5}, import.meta.url).text();
export const test = FileAttachment({"name":"../test.txt","mimeType":"text/plain","path":"../_file/test.f2ca1bb6.txt"/* observablehq-file */,"lastModified":/* ts */1706742000000,"size":5, import.meta.url).text();
Fil marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 3 additions & 3 deletions test/output/build/embed/_import/chart.2ce91e05.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import {FileAttachment} from "../_observablehq/stdlib.00000003.js";
import * as Plot from "../_npm/@observablehq/[email protected]/cd372fb8.js";
import {FileAttachment} from "../_observablehq/stdlib.00000003.js"/* observablehq-file */;
import * as Plot from "../_npm/@observablehq/[email protected]/cd372fb8.js"/* observablehq-file */;

export async function Chart() {
const gistemp = await FileAttachment({"name":"../lib/gistemp.csv","mimeType":"text/csv","path":"../_file/lib/gistemp.1cf298b1.csv","lastModified":/* ts */1706742000000,"size":97}, import.meta.url).csv({typed: true});
const gistemp = await FileAttachment({"name":"../lib/gistemp.csv","mimeType":"text/csv","path":"../_file/lib/gistemp.1cf298b1.csv"/* observablehq-file */,"lastModified":/* ts */1706742000000,"size":97, import.meta.url).csv({typed: true});
return Plot.plot({
y: {grid: true},
color: {scheme: "burd"},
Expand Down
8 changes: 4 additions & 4 deletions test/output/build/embed/chart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import "./_observablehq/stdlib.00000003.js";
import "./_npm/@observablehq/[email protected]/cd372fb8.js";
import "./_npm/[email protected]/cd372fb8.js";
export * from "./_import/chart.2ce91e05.js";
import "./_observablehq/stdlib.00000003.js"/* observablehq-file */;
import "./_npm/@observablehq/[email protected]/cd372fb8.js"/* observablehq-file */;
import "./_npm/[email protected]/cd372fb8.js"/* observablehq-file */;
export * from "./_import/chart.2ce91e05.js"/* observablehq-file */;
6 changes: 3 additions & 3 deletions test/output/build/fetches/_import/foo/foo.666599bc.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions test/output/build/fetches/_import/top.c85e149a.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion test/output/build/imports/_import/bar/bar.13bb8056.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export {bar} from "./baz.cdbfb28b.js";
export {bar} from "./baz.cdbfb28b.js"/* observablehq-file */;
2 changes: 1 addition & 1 deletion test/output/build/imports/_import/bar/baz.cdbfb28b.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {foo} from "../foo/foo.5963bf78.js";
import {foo} from "../foo/foo.5963bf78.js"/* observablehq-file */;

export const bar = "bar";
export const foobar = foo + "bar";
6 changes: 3 additions & 3 deletions test/output/build/imports/_import/foo/foo.5963bf78.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import "../../_npm/[email protected]/cd372fb8.js";
import {bar} from "../bar/bar.13bb8056.js";
export {top} from "../top.160847a6.js";
import "../../_npm/[email protected]/cd372fb8.js"/* observablehq-file */;
import {bar} from "../bar/bar.13bb8056.js"/* observablehq-file */;
export {top} from "../top.160847a6.js"/* observablehq-file */;

export const foo = "foo";
export const foobar = "foo" + bar;
2 changes: 1 addition & 1 deletion test/output/build/npm/_import/index.4bdc071f.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
import "../_observablehq/stdlib/inputs.00000005.js";
import "../_observablehq/stdlib/inputs.00000005.js"/* observablehq-file */;