remove _file #257

Fil · 2023-11-28T18:55:57Z

closes #203

(We can do _import in a follow-up PR.)

Fil · 2023-11-28T18:59:21Z

src/preview.ts

+        // This handles a static file.
+        try {
+          await access(path, constants.R_OK);
+          if ((await stat(path)).isFile()) {


this test is new… otherwise javascript was detected as the javascript/ directory

trebor

just the one warning otherwise this looks good to me.

src/preview.ts

trebor

:)

src/javascript/fetches.ts

src/preview.ts

mbostock · 2023-11-28T21:01:17Z

src/build.ts

@@ -73,7 +73,7 @@ export async function build({sourceRoot, outputRoot, verbose = true, addPublic =
  // Copy over the referenced files.
  for (const file of files) {
    let sourcePath = join(sourceRoot, file);
-    const outputPath = join(outputRoot, "_file", file);
+    const outputPath = join(outputRoot, file);


Should we error if the file already exists? (Which shouldn’t have been possible before, but should now be possible if you have e.g. an index.html and an index.md.)

We should not error, but we must definitely skip the file in that case.

Here are examples (admittedly fringe) where a file points to a generated page:

javascript.md:
<a href=javascript.html download>download this page</a>

or
<a href=javascript download>download this page</a>

now skipping both of these

mbostock

Approved. But please drop the access check and the TODO simplify comments.

see #257 (comment)

mbostock · 2023-11-29T14:37:04Z

test/promote-file-attachment-test.ts

Normally we name test files to match the source file that they are testing. It seems to me this should be in test/markdown-test.ts, and that the test should be named normalizePieceHtml, since that’s the imported function that is being tested. This is currently described as “file attachments” and “added” which doesn’t precisely indicate the function being tested.

mbostock · 2023-11-29T14:38:13Z

test/promote-file-attachment-test.ts

-      const htmlStr = html`<img src="./test.png">`;
-      const expected = html`<img src="./_file/test.png">`;
+      const htmlStr = html`<img src="./test.png"><img src="test.png">`;
+      const expected = html`<span><img src="./test.png"><img src="./test.png"></span>`;


Is this testing that file paths are normalized and not listed twice? This is a good thing to mention in the test description so that we remember the intent of this test going forward.

mbostock · 2023-11-29T14:43:47Z

src/build.ts

@@ -72,8 +72,9 @@ export async function build({sourceRoot, outputRoot, verbose = true, addPublic =

  // Copy over the referenced files.
  for (const file of files) {
+    const outputPath = join(outputRoot, file);
+    if (existsSync(outputPath) || existsSync(outputPath + ".html")) continue; // skip pages


I don’t think we always want to add .html to the path here? Shouldn’t that only apply in extension-less cases? I feel like we are doing too much work to support this extreme edge case of linking to the page itself to download source code. Can we just remove the .html check entirely please?

mbostock · 2023-11-29T14:53:56Z

src/markdown.ts

@@ -323,6 +323,7 @@ function renderIntoPieces(renderer: Renderer, root: string, sourcePath: string):
 }

 const SUPPORTED_PROPERTIES: readonly {query: string; src: "href" | "src" | "srcset"}[] = Object.freeze([
+  {query: "a[href]", src: "href"},


Suggested change

{query: "a[href]", src: "href"},

{query: "a[download][href]", src: "href"},

Seems to me that we shouldn’t promote links to file attachments unless they are explicitly marked as downloads.

Philosophically, I don’t expect this functionality to work transparently. Meaning: we have to document which elements get this special treatment, and users will have to read the documentation to know which elements are handled automatically and which aren’t — we can’t guarantee that this will work. The only way to guarantee that it will work is for the user to declare the file attachment statically using FileAttachment in JavaScript. For example, we’re not including object[data] in this list, which looks like this

<object type="video/mp4" data="path/flower.webm" width="600" height="140"> <img src="path/image.jpg" alt="useful image description"> </object>

I don’t think we should try to get everything here, and I don’t think we should feel “forced” to removing /_file to support promotion of linked files via a[href] — though I’m not really sure I follow why that solves a problem. And I do want to add content hashing of file attachments #260, so I’m not sure it is solving something.

And come to think of it, a more practical and reliable solution would to be have a public folder where everything gets copied #169, which we can offer in addition to promoting source files (and generated files) through static analysis. So then if you want to ensure that something is including in dist even if it’s not detectable through static analysis, you just drop it into public.

Not sure about limiting to a[download]; I mean, it's fine to add a[download] to the list, but my main motivation was more on the side of the [view source](page.md) pattern, which is otherwise a bit difficult to create with file attachments. I'll probably come back to it in some other way—on its own it doesn't warrant such a big change in any case.

FWIW I think the most useful incarnation of [view source](page.md) will be to link to the editable version on the Observable platform or GitHub etc, where people will not only be able to view the source but contribute changes.

Fil · 2023-11-29T16:42:56Z

There are several elements in this PR; I'll split them into smaller chunks (and maybe get back to removing _file later). One important thing will be to clarify the routing precedence.

remove _file

fad6fd5

Fil requested review from mbostock and trebor November 28, 2023 18:55

Merge branch 'main' into fil/remove-_file

d906ef4

Fil marked this pull request as draft November 28, 2023 18:56

Fil commented Nov 28, 2023

View reviewed changes

Fil marked this pull request as ready for review November 28, 2023 19:01

add a test for a more interesting path ../another/file

8a92435

trebor reviewed Nov 28, 2023

View reviewed changes

src/preview.ts Outdated Show resolved Hide resolved

Fil commented Nov 28, 2023

View reviewed changes

src/preview.ts Outdated Show resolved Hide resolved

Update src/preview.ts

5fafaa8

trebor approved these changes Nov 28, 2023

View reviewed changes

mbostock reviewed Nov 28, 2023

View reviewed changes

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

mbostock reviewed Nov 28, 2023

View reviewed changes

src/preview.ts Outdated Show resolved Hide resolved

mbostock reviewed Nov 28, 2023

View reviewed changes

mbostock approved these changes Nov 28, 2023

View reviewed changes

Fil mentioned this pull request Nov 29, 2023

promote local media assets to file attachments #250

Merged

Fil added 6 commits November 29, 2023 11:14

Merge branch 'main' into fil/remove-_file

9e12205

add a[href]

73cb6d5

fix tests

71a6f63

remove comment

e28c2d5

skip both page and page.html

453d0cf

see #257 (comment)

remove spurious test & update comments

e7998b2

mbostock reviewed Nov 29, 2023

View reviewed changes

clarify routing priorities

3dfe78f

Fil mentioned this pull request Nov 29, 2023

Redirect /subdir to /subdir/ when the page is given by docs/subdir/index.md #274

Merged

Fil closed this Nov 29, 2023

Fil mentioned this pull request Nov 29, 2023

move tests #277

Merged

Fil deleted the fil/remove-_file branch December 5, 2023 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove _file #257

remove _file #257

Fil commented Nov 28, 2023 •

edited

Loading

Fil Nov 28, 2023

trebor left a comment

trebor left a comment

mbostock Nov 28, 2023

Fil Nov 29, 2023

Fil Nov 29, 2023

mbostock left a comment

mbostock Nov 29, 2023

mbostock Nov 29, 2023

mbostock Nov 29, 2023

mbostock Nov 29, 2023

mbostock Nov 29, 2023

Fil Nov 29, 2023 •

edited

Loading

mbostock Nov 29, 2023

Fil commented Nov 29, 2023

	{query: "a[href]", src: "href"},
	{query: "a[download][href]", src: "href"},

remove _file #257

remove _file #257

Conversation

Fil commented Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

trebor left a comment

Choose a reason for hiding this comment

trebor 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

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

Fil Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fil commented Nov 29, 2023

Fil commented Nov 28, 2023 •

edited

Loading

Fil Nov 29, 2023 •

edited

Loading