-
Notifications
You must be signed in to change notification settings - Fork 193
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.
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
remove _file #257
Changes from all commits
fad6fd5
d906ef4
8a92435
5fafaa8
9e12205
73cb6d5
71a6f63
e28c2d5
453d0cf
e7998b2
3dfe78f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don’t think we always want to add |
||
| let sourcePath = join(sourceRoot, file); | ||
| const outputPath = join(outputRoot, "_file", file); | ||
| if (!existsSync(sourcePath)) { | ||
| const loader = Loader.find(sourceRoot, file); | ||
| if (!loader) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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"}, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 <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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about limiting to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I think the most useful incarnation of |
||||||
| {query: "audio[src]", src: "src"}, | ||||||
| {query: "audio source[src]", src: "src"}, | ||||||
| {query: "img[src]", src: "src"}, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,28 +95,6 @@ export class PreviewServer { | |
| throw new HttpError("Not found", 404); | ||
| } | ||
| end(req, res, rewriteModule(js, file, createImportResolver(this.root)), "text/javascript"); | ||
| } else if (pathname.startsWith("/_file/")) { | ||
| const path = pathname.slice("/_file".length); | ||
| const filepath = join(this.root, path); | ||
| try { | ||
| await access(filepath, constants.R_OK); | ||
| send(req, pathname.slice("/_file".length), {root: this.root}).pipe(res); | ||
| return; | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
|
|
||
| // Look for a data loader for this file. | ||
| const loader = Loader.find(this.root, path); | ||
| if (loader) { | ||
| try { | ||
| send(req, await loader.load(), {root: this.root}).pipe(res); | ||
| return; | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
| } | ||
| throw new HttpError("Not found", 404); | ||
| } else { | ||
| if ((pathname = normalize(pathname)).startsWith("..")) throw new Error("Invalid path: " + pathname); | ||
| let path = join(this.root, pathname); | ||
|
|
@@ -134,23 +112,19 @@ export class PreviewServer { | |
| try { | ||
| if ((await stat(path)).isDirectory() && (await stat(join(path, "index") + ".md")).isFile()) { | ||
| await access(join(path, "index") + ".md", constants.R_OK); | ||
| if (!path.endsWith("/")) { | ||
| res.writeHead(302, {Location: pathname + "/" + url.search}); | ||
| res.end(); | ||
| return; | ||
| } | ||
| pathname = join(pathname, "index"); | ||
| path = join(path, "index"); | ||
| } | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; // internal error | ||
| } | ||
|
|
||
| // If this path ends with .html, then redirect to drop the .html. TODO: | ||
| // Check for the existence of the .md file first. | ||
| if (extname(path) === ".html") { | ||
| res.writeHead(302, {Location: join(dirname(pathname), basename(pathname, ".html")) + url.search}); | ||
| res.end(); | ||
| return; | ||
| } | ||
|
|
||
| // Otherwise, serve the corresponding Markdown file, if it exists. | ||
| // Anything else should 404; static files should be matched above. | ||
| // Serve the corresponding Markdown file, if it exists. | ||
| try { | ||
| const config = await readConfig(this.root); | ||
| const {html} = await renderPreview(await readFile(path + ".md", "utf-8"), { | ||
|
|
@@ -160,10 +134,47 @@ export class PreviewServer { | |
| ...config | ||
| }); | ||
| end(req, res, html, "text/html"); | ||
| return; | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; // internal error | ||
| throw new HttpError("Not found", 404); | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
|
|
||
| // Handle a static file. | ||
| try { | ||
| if ((await stat(path)).isFile()) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
| } | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
|
|
||
| // If this path ends with .html and a .md file exists, redirect. | ||
| if (extname(path) === ".html") { | ||
| try { | ||
| if ((await stat(path.slice(0, -".html".length) + ".md")).isFile()) { | ||
| res.writeHead(302, {Location: join(dirname(pathname), basename(pathname, ".html")) + url.search}); | ||
| res.end(); | ||
| return; | ||
| } | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
| } | ||
|
|
||
| // Look for a data loader for this file. | ||
| const loader = Loader.find(this.root, pathname); | ||
| if (loader) { | ||
| try { | ||
| send(req, await loader.load(), {root: this.root}).pipe(res); | ||
| return; | ||
| } catch (error) { | ||
| if (!isEnoent(error)) throw error; | ||
| } | ||
| } | ||
|
|
||
| // Otherwise, 404. | ||
| throw new HttpError("Not found", 404); | ||
| } | ||
| } catch (error) { | ||
| console.error(error); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Hola,mundo | ||
| 1,2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Hola,mundo | ||
| 1,2 |
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.
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.htmland anindex.md.)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.
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.
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