-
Notifications
You must be signed in to change notification settings - Fork 118
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
annotate local paths #1731
base: main
Are you sure you want to change the base?
annotate local paths #1731
Conversation
src/javascript/transpile.ts
Outdated
* 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be isAssetPath
or whatever that’s ['./', '../', '/']
. Don’t want to match against .foo
.
Sorry for the delay. I plan to review this tomorrow. |
@@ -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 */;'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will work. Thanks!
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
in lieu of
The annotate.js scripts lists a few cases that can be annotated.