Support relative imports #40

cinxmo · 2023-10-20T21:57:05Z

Resolves #59, resolves #30

Description

The purpose of this PR is to:

1. Support relative imports

Users want to be able to define their own JS modules and use them throughout a JS cell. For example, if we have:

nested page (/docs/subDocs/javascript.md)
a helper function in /docs/subDocs/bar.js
in /docs/subDocs/javascript.md we want to import {bar} from "./bar.js"
Prior to this change, it assumed "./bar.js" is relative to the root directory (/docs). With this change, it will recognize that "./" is relative to /docs/subDocs/

To have this work, we pass in the sourcePath, i.e. the path relative to the root excluding the root. For example, in /docs/subDocs/javascript.md the root is docs and the sourcePath is subDocs/javascript.md. The resulting import "path" would be subDocs/bar.js (as opposed to /bar.js since we no longer assume it is sourced relative to the root). This ensures that as we're building the list of imports, the path is always going to be relative to the root, but excludes the root.

2. Promote local imports and local fetches to file attachment references

At the same time, we want to promote local imports (and fetches) to file attachment references. These files are copied over as part of the source files for the production build. ~~They are also watched as part of incremental reloading.~~

Testing

write test cases

src/javascript/imports.ts

src/navigation.ts

src/markdown.ts

src/javascript/imports.ts

mbostock · 2023-10-23T21:53:04Z

Ref. #30.

mbostock

Thanks, Cindy! Here’s a summary of my suggested changes:

Let’s fold the generation of FileAttachment features for imports into findImports instead of doing it in two passes (first in findImports and second in findFeatures). This should fix the cross-product logic bug.
Instead of “baking” the source root into the file paths, let’s favor paths within the root. This should eliminate the need to check that the sourcePath starts with the root in getPathFromRoot (it should be required for any local path), and the subsequent need to slice off the root prefix, and the need to pass the root down to the client etc.
Let’s make the sourcePath argument required instead of optional. It’s needed to compute the correct output, so we shouldn’t allow it to be missing.
Tests! I highly, highly encourage you to write unit tests during the development of this feature, not just as a chore after you’ve written it. Writing tests alongside code will give you better feedback as you make changes, and will ensure that the logic is fully covered by tests going forward.

src/javascript.ts

mbostock · 2023-10-29T16:24:24Z

src/javascript.ts

-  const features = findFeatures(body, references, input);
-  const imports = findImports(body, root);
+  const imports = findImports(body, root, sourcePath);
+  const features = findFeatures(body, root, sourcePath, imports, references, input);


This is a lot of positional arguments. Especially in JavaScript where we lack type protection, we want to avoid too many positional arguments because it’s easy to get the order wrong or skip one, and cause unexpectede behavior. It probably makes sense to switch to an options object with named arguments at this point.

But see also the comment below: I think we probably don’t need to pass root if we instead redefine sourcePath so that it is a subpath of the root. I.e., instead of passing root = docs and sourcePath = docs/index.md, we say sourcePath = index.md which is within docs. (There should never be a source path outside of the root.) And I recommend we fold the imports file attachments logic into findImports anyway.

mbostock · 2023-10-29T16:44:18Z

src/javascript/features.js

+        features.push({type: "FileAttachment", name: pathFromRoot});
+        // add transitive imports
+        features.push(
+          ...imports.filter((im) => im.name !== pathFromRoot).map((im) => ({type: "FileAttachment", name: im.name}))


It looks like this will generate a lot of duplicate file attachment features. That may not ultimately matter since the files will subsequently get de-duplicated, but I think we can fix this logic.

The imports array is generated by findImports and passed in. This array consists of all the (statically-analyzable) imported modules, either as bare specifiers (e.g., npm:d3) or URLs (e.g., https://cdn.jsdelivr.net/npm/d3/+esm) or relative paths (e.g., ./foo/bar.js). And it includes both top-level imports from the JavaScript and transitive imports from imported local modules.

Here, we’re walking over the AST looking for ImportExpression and ImportDeclaration nodes. We’re only considering the top-level imports. And we’re outputting everything in the passed-in imports except for that one that matches the computed pathFromRoot (i.e., the current top-level import). But if the code has multiple imports, and each of those imports has transitive imports, this effectively computes the cross product of imports so there will be many redundant entries in the output.

Also, stepping back for a second, this is fixing an unfiled bug: our current logic for promoting local imports to file attachments only considers top-level imports, not transitive imports. I filed #76 to track this bug.

I think the logic would be simpler if we consolidated this logic into findImports, i.e., we should have findImports compute the array of imports but also append file attachments to features at the same time so that we don’t have to traverse twice.

src/javascript/imports.ts

mbostock · 2023-10-29T17:17:18Z

src/render.ts

-  .concat(parseResult.imports.filter(({name}) => name.startsWith("./")).map(({name}) => `/_file/${name.slice(2)}`))
+  .concat(
+    parseResult.imports
+      .filter(({name}) => name.startsWith(root))


We don’t want to push this root down to the client — this is the relative path from the cwd to the source root, and it doesn’t need to be exposed to the client. (It’s only needed by the server.) Instead, we should have the server normalize the local paths so that they always start with a / and are relative to the root. For example, if the root is docs and we’re rendering docs/eng/github.md and the code imports ../chart.js, the path should be normalized to /chart.js. Then here we just need to check name.startsWith("/") for local imports.

src/render.ts

src/javascript/helpers.js

src/javascript/imports.ts

…indImports

cinxmo · 2023-10-30T01:14:21Z

src/javascript.ts

@@ -105,8 +117,8 @@ export function parseJavaScript(input: string, options: ParseOptions) {
  const body = expression ?? (Parser.parse(input, parseOptions) as any);
  const references = findReferences(body, globals, input);
  const declarations = expression ? null : findDeclarations(body, globals, input);
-  const features = findFeatures(body, references, input);
-  const imports = findImports(body, root);
+  const {imports, features: importFeatures} = findImports(body, root, sourcePath);


don't really like how it returns features and imports in a function called "findImports" 🤔

We will also have additional features like Secrets soon, so we should anticipate that if we can.

Filed a ticket to address the confusion around adding local fetches and local imports as file attachments: #87

src/javascript.ts

wiltsecarpenter · 2023-10-30T16:51:51Z

src/javascript.ts

@@ -105,8 +117,8 @@ export function parseJavaScript(input: string, options: ParseOptions) {
  const body = expression ?? (Parser.parse(input, parseOptions) as any);
  const references = findReferences(body, globals, input);
  const declarations = expression ? null : findDeclarations(body, globals, input);
-  const features = findFeatures(body, references, input);
-  const imports = findImports(body, root);
+  const {imports, features: importFeatures} = findImports(body, root, sourcePath);


We will also have additional features like Secrets soon, so we should anticipate that if we can.

wiltsecarpenter · 2023-10-30T16:52:44Z

src/javascript/features.js


-export function findFeatures(node, references, input) {
+export function findFeatures(node, sourcePath, references, input) {


In the higher level code with have "root" and "path", how does "sourcePath" relate to those? Maybe there's a more descriptive term?

the definition of sourcePath should be global to the codebase. @cinxmo document it

I think we should have a larger discussion around documentation, especially how to document function arguments. I like using docstrings

src/javascript/imports.ts

wiltsecarpenter · 2023-10-30T16:58:51Z

src/javascript/imports.ts

@@ -83,7 +90,9 @@ export function rewriteImports(output, root) {
              : node.specifiers.some(isNamespaceSpecifier)
              ? node.specifiers.find(isNamespaceSpecifier).local.name
              : "{}"
-          } = await import(${value.startsWith("./") ? JSON.stringify("/_file/" + value.slice(2)) : node.source.raw});`
+          } = await import(${
+            isLocalImport(value) ? JSON.stringify(join("/_file/", join(dirname(sourcePath), value))) : node.source.raw


I would define a constant for "/_file" somewhere and use that so it is easier to find all the places where we use it.

Yes, and eventually we’ll have to support a serving root prefix #42 too.

I'd like to address this in a separate PR along with #42

mbostock · 2023-10-31T21:39:47Z

src/render.ts

-      p.path.replace(/\/index$/, "/")
-    )}">${escapeData(p.name)}</a></li>`
+    <li class="observablehq-link${
+      p.path === sourcePath ? " observablehq-link-active" : ""


I think either this comparison is wrong or the name sourcePath is inappropriate here.

The p.path here comes from readPages and represents the serving path. We use sourcePath to refer to a source path within the source root. The difference is that a serving path does not include the file extension .md at the end whereas a source path does.

https://github.com/observablehq/cli/blob/7a22ae48dbd201cf1947b8594f870789ed956096/src/navigation.ts#L20-L21

Is sourcePath a source path here, or is it the serving path?

sorry I lazily changed it without realizing it broke the highlighting. it should be the serving path (so source path without the .md extension)

mbostock · 2023-10-31T21:42:51Z

src/render.ts

@@ -63,7 +64,7 @@ ${
 ${JSON.stringify({imports: Object.fromEntries(Array.from(imports, ([name, href]) => [name, href]))}, null, 2)}
 </script>
 ${Array.from(imports.values())
-  .concat(parseResult.imports.filter(({name}) => name.startsWith("./")).map(({name}) => `/_file/${name.slice(2)}`))
+  .concat(parseResult.imports.filter(({type}) => type === "local").map(({name}) => join("/_file/", name)))


The behavior of path.join depends on the operating system: it uses the platform-specific path separator. When constructing a URL, we always want to use a forward slash, so we should avoid using path.join here. Is the name here normalized so that it always starts with a leading forward slash?

name isn't normalized to start with a /. in fact, it shouldn't have a leading / because of how sourcePath is constructed. Changing to .map(({name}) => `/_file/${name}`)

actually no my previous statement was wrong. I think it's safer to remove the leading `/ if it exists

cinxmo · 2023-11-02T20:22:15Z

@wiltsecarpenter incremental reloading after making changes to a local ES module isn't working (unfortunately), but this is existing behavior. I'd like to merge this PR first and fix that in a separate one

cinxmo · 2023-11-03T22:14:47Z

with the latest changes to data loaders, this PR is broken. ~~trying to put in a fix now~~ fixed in latest commit

mbostock · 2023-11-07T16:23:56Z

src/render.ts

@@ -127,7 +126,7 @@ function getImportPreloads(parseResult: ParseResult): Iterable<string> {
  const preloads: string[] = [];
  for (const specifier of specifiers) {
    const resolved = resolveImport(specifier);
-    if (resolved.startsWith("/") || resolved.startsWith("https://")) {
+    if (resolved.startsWith("/_observablehq") || resolved.startsWith("https://")) {


I don’t think we have a test for preloads yet, but this change looks like it will break module preloading of imported local ES modules (whose paths start with /_file/).

Also, we would want to use /_observablehq/ here with a terminal slash to avoid a false positive on another (non-special) directory that coincidentally happens to start with _observablehq such as _observablehq2. (I don’t expect that to ever happen in practice, but we should write the code precisely just the same.)

I don't think it'll break the module preloading of imported local ES modules since they should be handled by this concatenation:
https://github.com/observablehq/cli/blob/b1cdf2f4982244b7b74d331b0dfaca90f2d3bc78/src/render.ts#L62-L69

This function could be renamed to getGlobalImportPreloads

Ah, sorry, I missed that. I would prefer to keep it getImportPreloads and move the local preloads back into the function.

cinxmo · 2023-11-07T17:21:28Z

test/isLocalImport-test.ts

+  return join(root + "/", dirname(sourcePath), value);
+}
+
+describe("isLocalImport", () => {


@wiltsecarpenter I modified isLocalImport and added some test cases. let me know what you think

mbostock

Very close! Just one blocker on the getImportPreloads.

mbostock · 2023-11-07T21:20:54Z

src/render.ts

@@ -122,7 +118,7 @@ function getImportPreloads(parseResult: ParseResult): Iterable<string> {
  const preloads: string[] = [];
  for (const specifier of specifiers) {
    const resolved = resolveImport(specifier);
-    if (resolved.startsWith("/") || resolved.startsWith("https://")) {
+    if (resolved.startsWith("/_observablehq/") || resolved.startsWith("https://")) {


I think we still want to remove this change. We added the local import to specifiers above, but it needs to be added to preloads here.

test/input/imports/bar.js

mbostock · 2023-11-07T21:22:28Z

test/javascript-test.ts

+  const IMPORT_TEST_CASES = "./test/input/imports";
+  const IMPORT_TEST_RESULTS = "./test/output/imports";
+  for (const name of readdirSync(IMPORT_TEST_CASES)) {
+    if (!isJsFile(IMPORT_TEST_CASES, name) || !name.includes("import")) continue;


If it’s not too much work, could we consolidate code with the other tests above? This looks like they’re doing almost the same thing.

This code was broken out to test nested/transitive imports. We can look at combining the test cases in the future

One of the things missing from the copied version is generating the -changed.js when the test fails. When a test fails, you’ll see an error, e.g.,

AssertionError [ERR_ASSERTION]: dynamic-import.js must match snapshot

but there’s no way to debug the error because the actual output of the test isn’t saved to disk or shown in the console.

Co-authored-by: Mike Bostock <[email protected]>

cinxmo · 2023-11-07T23:18:12Z

I found (yet another) bug when testing with the existing code in cli-experiments. Steps to reproduce:

In the cli-experiments repo on the fil branch, update package.json to "@observablehq/cli": "https://github.com/observablehq/cli.git#fix-import-rewriting"
Run yarn to install this branch's changes
In the browser, navigate to http://127.0.0.1:3001/wine-navio
Notice the TypeError: Failed to fetch dynamically imported module: npm:[email protected]

The issue is that in a Markdown cell, we import:

import {navio} from "./navio.js";

and in ./navio.js we import an NPM package using the npm: specifier:

export async function navio(data, {columns = data.columns ?? Object.keys(data[0]), height = 300} = {}) {
  const navio = await import("npm:[email protected]").then((d) => d.default);
...

but we don't rewrite transitive imports as noted in one of our pairing sessions. Transitive imports need to be rewritten for this use case. I think we should handle this in a separate PR

mbostock · 2023-11-08T00:50:42Z

Transitive imports need to be rewritten for this use case. I think we should handle this in a separate PR

This is a regression I introduced in #101. To summarize:

If we use import maps, then you can use npm: protocol imports in local modules without rewriting; however since import maps cannot be modified after load, you have to reload the page whenever you add a new npm: protocol import or you get an error Detect when a library should be added to the import map during live preview #14.
If we don’t use import maps, then we have to rewrite local modules (and not just JavaScript in Markdown) to allow npm: protocol imports. I had forgotten this was my main motivation for adding import maps in the first place!

I’ll file an issue #115 for this. Thanks for finding this.

cinxmo requested review from mbostock, wiltsecarpenter and mythmon October 20, 2023 21:57

mbostock reviewed Oct 20, 2023

View reviewed changes

src/javascript/imports.ts Outdated Show resolved Hide resolved

mbostock reviewed Oct 20, 2023

View reviewed changes

src/navigation.ts Outdated Show resolved Hide resolved

mbostock reviewed Oct 20, 2023

View reviewed changes

src/markdown.ts Outdated Show resolved Hide resolved

mbostock reviewed Oct 20, 2023

View reviewed changes

src/javascript/imports.ts Outdated Show resolved Hide resolved

support relative imports

d13dbc8

cinxmo mentioned this pull request Oct 23, 2023

Fetching a path that starts with “../” or “/” is not automatically promoted to a file attachment #59

Closed

progress?

640db4f

cinxmo force-pushed the fix-import-rewriting branch from 66ad9d5 to 640db4f Compare October 23, 2023 23:51

Fil mentioned this pull request Oct 26, 2023

Don’t allow source files outside of the source root #71

Closed

cinxmo added 2 commits October 26, 2023 11:54

more progress part 2?

db1a051

Merge branch 'main' into fix-import-rewriting

6e976e7

cinxmo requested a review from mbostock October 29, 2023 16:20

mbostock mentioned this pull request Oct 29, 2023

Transitive imports are not identified as file attachments #76

Closed

mbostock requested changes Oct 29, 2023

View reviewed changes

cinxmo added 2 commits October 29, 2023 21:10

sourcePath within root, require sourcePath, add FileAttachment into f…

e8e5e3c

…indImports

delete unused constants.js

0d6033c

cinxmo commented Oct 30, 2023

View reviewed changes

update tests

1222cba

wiltsecarpenter reviewed Oct 30, 2023

View reviewed changes

address feedback and add tests

68456bb

cinxmo marked this pull request as ready for review October 31, 2023 20:43

Merge branch 'main' into fix-import-rewriting

909f798

cinxmo requested review from wiltsecarpenter and mbostock October 31, 2023 21:12

mbostock reviewed Oct 31, 2023

View reviewed changes

small fix

1a33113

cinxmo requested review from mbostock and wiltsecarpenter November 3, 2023 15:29

cinxmo added 2 commits November 3, 2023 11:30

changes

ae9486d

Merge branch 'main' into fix-import-rewriting

ea807d0

cinxmo added 4 commits November 3, 2023 18:47

do not throw error if file is not a data loader

0fc3aa3

revert unnecessary changes

e733072

Merge branch 'main' into fix-import-rewriting

f996813

address feedback, move static-import-npm to imports test directory

4d16092

cinxmo removed the request for review from mythmon November 7, 2023 16:17

mbostock reviewed Nov 7, 2023

View reviewed changes

add test cases for isLocalImport

b1cdf2f

cinxmo commented Nov 7, 2023

View reviewed changes

cinxmo added 3 commits November 7, 2023 12:30

add ending slash

fd9b860

Merge branch 'main' into fix-import-rewriting

be546da

add comment and modify getImportPreloads

00e064c

mbostock requested changes Nov 7, 2023

View reviewed changes

add local imports to preloads list

55d068d

wiltsecarpenter approved these changes Nov 7, 2023

View reviewed changes

Update test/input/imports/bar.js

22ce01e

Co-authored-by: Mike Bostock <[email protected]>

cinxmo requested a review from mbostock November 7, 2023 21:43

mbostock approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'main' into fix-import-rewriting

2ced0f6

cinxmo merged commit d8f5710 into main Nov 7, 2023
1 check passed

cinxmo deleted the fix-import-rewriting branch November 7, 2023 23:45

mbostock mentioned this pull request Nov 8, 2023

DRY tests #119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support relative imports #40

Support relative imports #40

cinxmo commented Oct 20, 2023 •

edited

Loading

mbostock commented Oct 23, 2023

mbostock left a comment

mbostock Oct 29, 2023

mbostock Oct 29, 2023 •

edited

Loading

mbostock Oct 29, 2023

cinxmo Oct 30, 2023

wiltsecarpenter Oct 30, 2023

cinxmo Nov 2, 2023

wiltsecarpenter Oct 30, 2023

wiltsecarpenter Oct 30, 2023

cinxmo Oct 30, 2023

cinxmo Nov 2, 2023

wiltsecarpenter Oct 30, 2023

mbostock Oct 30, 2023

cinxmo Nov 1, 2023

mbostock Oct 31, 2023

cinxmo Nov 1, 2023

mbostock Oct 31, 2023

cinxmo Nov 1, 2023

cinxmo Nov 1, 2023

cinxmo commented Nov 2, 2023

cinxmo commented Nov 3, 2023 •

edited

Loading

mbostock Nov 7, 2023

cinxmo Nov 7, 2023

mbostock Nov 7, 2023

cinxmo Nov 7, 2023

mbostock left a comment

mbostock Nov 7, 2023

cinxmo Nov 7, 2023

mbostock Nov 7, 2023

cinxmo Nov 7, 2023

mbostock Nov 8, 2023

cinxmo commented Nov 7, 2023 •

edited

Loading

mbostock commented Nov 8, 2023 •

edited

Loading


		export function findFeatures(node, references, input) {
		export function findFeatures(node, sourcePath, references, input) {

Support relative imports #40

Support relative imports #40

Conversation

cinxmo commented Oct 20, 2023 • edited Loading

Description

1. Support relative imports

2. Promote local imports and local fetches to file attachment references

Testing

mbostock commented Oct 23, 2023

mbostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbostock Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cinxmo commented Nov 2, 2023

cinxmo commented Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbostock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cinxmo commented Nov 7, 2023 • edited Loading

mbostock commented Nov 8, 2023 • edited Loading

cinxmo commented Oct 20, 2023 •

edited

Loading

mbostock Oct 29, 2023 •

edited

Loading

cinxmo commented Nov 3, 2023 •

edited

Loading

cinxmo commented Nov 7, 2023 •

edited

Loading

mbostock commented Nov 8, 2023 •

edited

Loading