Conversation
| // This handles a static file. | ||
| try { | ||
| await access(path, constants.R_OK); | ||
| if ((await stat(path)).isFile()) { |
There was a problem hiding this comment.
this test is new… otherwise javascript was detected as the javascript/ directory
trebor
left a comment
There was a problem hiding this comment.
just the one warning otherwise this looks good to me.
| for (const file of files) { | ||
| let sourcePath = join(sourceRoot, file); | ||
| const outputPath = join(outputRoot, "_file", file); | ||
| const outputPath = join(outputRoot, file); |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
now skipping both of these
mbostock
left a comment
There was a problem hiding this comment.
Approved. But please drop the access check and the TODO simplify comments.
There was a problem hiding this comment.
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.
| 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>`; |
There was a problem hiding this comment.
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.
| // Copy over the referenced files. | ||
| for (const file of files) { | ||
| const outputPath = join(outputRoot, file); | ||
| if (existsSync(outputPath) || existsSync(outputPath + ".html")) continue; // skip pages |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| const SUPPORTED_PROPERTIES: readonly {query: string; src: "href" | "src" | "srcset"}[] = Object.freeze([ | ||
| {query: "a[href]", src: "href"}, |
There was a problem hiding this comment.
| {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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
There are several elements in this PR; I'll split them into smaller chunks (and maybe get back to removing |
closes #203
(We can do _import in a follow-up PR.)