Skip to content

remove _file #257

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

Closed
Fil wants to merge 11 commits into from
Closed

remove _file #257

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fad6fd5
remove _file
Fil Nov 28, 2023
d906ef4
Merge branch 'main' into fil/remove-_file
Fil Nov 28, 2023
8a92435
add a test for a more interesting path ../another/file
Fil Nov 28, 2023
5fafaa8
Update src/preview.ts
Fil Nov 28, 2023
9e12205
Merge branch 'main' into fil/remove-_file
Fil Nov 29, 2023
73cb6d5
add a[href]
Fil Nov 29, 2023
71a6f63
fix tests
Fil Nov 29, 2023
e28c2d5
remove comment
Fil Nov 29, 2023
453d0cf
skip both page and page.html
Fil Nov 29, 2023
e7998b2
remove spurious test & update comments
Fil Nov 29, 2023
3dfe78f
clarify routing priorities
Fil Nov 29, 2023
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
3 changes: 2 additions & 1 deletion src/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@ 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);
Copy link
Copy Markdown
Member

@mbostock mbostock Nov 28, 2023

Choose a reason for hiding this comment

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

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.)

Copy link
Copy Markdown
Contributor Author

@Fil Fil Nov 29, 2023

Choose a reason for hiding this comment

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

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>

Copy link
Copy Markdown
Contributor Author

@Fil Fil Nov 29, 2023

Choose a reason for hiding this comment

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

now skipping both of these

if (existsSync(outputPath) || existsSync(outputPath + ".html")) continue; // skip pages
Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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?

let sourcePath = join(sourceRoot, file);
if (!existsSync(sourcePath)) {
const loader = Loader.find(sourceRoot, file);
if (!loader) {
Expand Down
2 changes: 1 addition & 1 deletion src/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function fileReference(name: string, sourcePath: string): FileReference {
return {
name,
mimeType: mime.getType(name),
path: relativeUrl(sourcePath, resolvePath(sourcePath, name)) // TODO simplify?
path: relativeUrl(sourcePath, resolvePath(sourcePath, name))
};
}

Expand Down
2 changes: 1 addition & 1 deletion src/javascript/fetches.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export function rewriteFetches(output: Sourcemap, rootNode: JavaScriptNode, sour
if (isLocalFetch(node, rootNode.references, sourcePath)) {
const arg = node.arguments[0];
const value = getStringLiteralValue(arg);
const path = relativeUrl(sourcePath, resolvePath(sourcePath, value)); // TODO simplify?
const path = relativeUrl(sourcePath, resolvePath(sourcePath, value));
output.replaceLeft(arg.start, arg.end, JSON.stringify(path));
}
}
Expand Down
1 change: 1 addition & 0 deletions src/markdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@Fil Fil Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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.

{query: "audio[src]", src: "src"},
{query: "audio source[src]", src: "src"},
{query: "img[src]", src: "src"},
Expand Down
5 changes: 2 additions & 3 deletions src/preview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,8 @@ export class PreviewServer {
return;
}

// This handles a static file.
// Handle a static file.
try {
await access(path, constants.R_OK);
if ((await stat(path)).isFile()) {
Copy link
Copy Markdown
Contributor Author

@Fil Fil Nov 28, 2023

Choose a reason for hiding this comment

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

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

send(req, pathname, {root: this.root}).pipe(res);
return;
Expand All @@ -150,7 +149,7 @@ export class PreviewServer {
}

// Otherwise, serve the corresponding Markdown file, if it exists.
// Anything else should 404; static files should be matched above.
// Anything else should 404.
try {
const config = await readConfig(this.root);
const {html} = await renderPreview(await readFile(path + ".md", "utf-8"), {
Expand Down
42 changes: 21 additions & 21 deletions test/promote-file-attachment-test.ts
View file
Open in desktop
Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ describe("file attachments", () => {
const sourcePath = "/attachments.md";

it("img[src]", () => {
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>`;
Copy link
Copy Markdown
Member

@mbostock mbostock Nov 29, 2023

Choose a reason for hiding this comment

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

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.

const context = mockContext();
const actual = normalizePieceHtml(htmlStr, sourcePath, context);

Expand All @@ -19,7 +19,7 @@ describe("file attachments", () => {
{
mimeType: "image/png",
name: "./test.png",
path: "./_file/test.png"
path: "./test.png"
}
]);
});
Expand All @@ -35,8 +35,8 @@ describe("file attachments", () => {
/>
`;
const expected = html`
<img srcset="./_file/small.jpg 480w, ./_file/large.jpg 800w" sizes="(max-width: 600px) 480px,
800px" src="./_file/large.jpg" alt="Image for testing">
<img srcset="./small.jpg 480w, ./large.jpg 800w" sizes="(max-width: 600px) 480px,
800px" src="./large.jpg" alt="Image for testing">
`;
const context = mockContext();
const actual = normalizePieceHtml(htmlStr, sourcePath, context);
Expand All @@ -46,12 +46,12 @@ describe("file attachments", () => {
{
mimeType: "image/jpeg",
name: "large.jpg",
path: "./_file/large.jpg"
path: "./large.jpg"
},
{
mimeType: "image/jpeg",
name: "small.jpg",
path: "./_file/small.jpg"
path: "./small.jpg"
}
]);
});
Expand All @@ -60,7 +60,7 @@ describe("file attachments", () => {
const htmlStr = html`<video src="observable.mov" controls>
Your browser doesn't support HTML video.
</video>`;
const expected = html`<video src="./_file/observable.mov" controls>
const expected = html`<video src="./observable.mov" controls>
Your browser doesn't support HTML video.
</video>`;
const context = mockContext();
Expand All @@ -71,7 +71,7 @@ describe("file attachments", () => {
{
mimeType: "video/quicktime",
name: "observable.mov",
path: "./_file/observable.mov"
path: "./observable.mov"
}
]);
});
Expand All @@ -84,8 +84,8 @@ describe("file attachments", () => {
</video>`;

const expected = html`<video width="320" height="240" controls>
<source src="./_file/observable.mp4" type="video/mp4">
<source src="./_file/observable.mov" type="video/mov">
<source src="./observable.mp4" type="video/mp4">
<source src="./observable.mov" type="video/mov">
Your browser doesn't support HTML video.
</video>`;

Expand All @@ -97,12 +97,12 @@ describe("file attachments", () => {
{
mimeType: "video/mp4",
name: "observable.mp4",
path: "./_file/observable.mp4"
path: "./observable.mp4"
},
{
mimeType: "video/quicktime",
name: "observable.mov",
path: "./_file/observable.mov"
path: "./observable.mov"
}
]);
});
Expand All @@ -114,8 +114,8 @@ describe("file attachments", () => {
</picture>`;

const expected = html`<picture>
<source srcset="./_file/observable-logo-wide.png" media="(min-width: 600px)">
<img src="./_file/observable-logo-narrow.png">
<source srcset="./observable-logo-wide.png" media="(min-width: 600px)">
<img src="./observable-logo-narrow.png">
</picture>`;

const context = mockContext();
Expand All @@ -126,12 +126,12 @@ describe("file attachments", () => {
{
mimeType: "image/png",
name: "observable-logo-narrow.png",
path: "./_file/observable-logo-narrow.png"
path: "./observable-logo-narrow.png"
},
{
mimeType: "image/png",
name: "observable-logo-wide.png",
path: "./_file/observable-logo-wide.png"
path: "./observable-logo-wide.png"
}
]);
});
Expand Down Expand Up @@ -160,7 +160,7 @@ describe("file attachments", () => {
/>
`;
const expected = html`
<img srcset="./_file/small.jpg 480w, https://upload.wikimedia.org/900px-American_Shorthair.jpg 900w" sizes="(max-width: 600px) 480px, 900px" src="https://upload.wikimedia.org/900px-American_Shorthair.jpg" alt="Cat image for testing">
<img srcset="./small.jpg 480w, https://upload.wikimedia.org/900px-American_Shorthair.jpg 900w" sizes="(max-width: 600px) 480px, 900px" src="https://upload.wikimedia.org/900px-American_Shorthair.jpg" alt="Cat image for testing">
`;
const context = mockContext();
const actual = normalizePieceHtml(htmlStr, sourcePath, context);
Expand All @@ -170,7 +170,7 @@ describe("file attachments", () => {
{
mimeType: "image/jpeg",
name: "small.jpg",
path: "./_file/small.jpg"
path: "./small.jpg"
}
]);
});
Expand All @@ -184,7 +184,7 @@ describe("file attachments", () => {

const expected = html`<video width="320" height="240" controls>
<source src="https://www.youtube.com/watch?v=SsFyayu5csc" type="video/youtube">
<source src="./_file/observable.mov" type="video/mov">
<source src="./observable.mov" type="video/mov">
Your browser doesn't support HTML video.
</video>`;

Expand All @@ -196,7 +196,7 @@ describe("file attachments", () => {
{
mimeType: "video/quicktime",
name: "observable.mov",
path: "./_file/observable.mov"
path: "./observable.mov"
}
]);
});
Expand Down