Skip to content
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

fix file path resolution #120

Merged
merged 21 commits into from
Nov 9, 2023
Merged

fix file path resolution #120

merged 21 commits into from
Nov 9, 2023

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Nov 8, 2023

Fixes #117. Fixes #123. Related #87 (comment).

@mbostock mbostock requested a review from cinxmo November 8, 2023 05:26
src/preview.ts Show resolved Hide resolved
src/javascript.ts Outdated Show resolved Hide resolved
@cinxmo

This comment was marked as resolved.

src/javascript.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@cinxmo cinxmo left a comment

Choose a reason for hiding this comment

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

requesting a few changes 🙏

Base automatically changed from mbostock/dry-javascript-tests to main November 8, 2023 16:31
Copy link
Member Author

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Okay! In the latest changes…

I really want to add more tests here, but I’m waiting for @wiltsecarpenter’s #102.

@mbostock mbostock requested a review from cinxmo November 8, 2023 18:32
@mbostock mbostock mentioned this pull request Nov 8, 2023
@cinxmo
Copy link
Contributor

cinxmo commented Nov 9, 2023

I am seeing socket closed and some interesting behavior when testing locally:

  1. I modified .observable/config to add one extra path {path: "/subDocs/testImports", name: "Test imports"}
  2. I added subDocs/testImports.md
Screen.Recording.2023-11-08.at.8.30.19.PM.mov

@@ -20,7 +20,7 @@ export async function readPages(root: string): Promise<NonNullable<RenderOptions
continue;
}
const name = basename(file, ".md");
const page = {path: `/${join(dirname(file), name)}`, name: parsed.title ?? "Untitled"};
const page = {path: join(dirname(file), name), name: parsed.title ?? "Untitled"};
Copy link
Contributor

@cinxmo cinxmo Nov 9, 2023

Choose a reason for hiding this comment

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

since navigation is for the serving path and join is for the source path, I think we'd want to keep the original logic? Is join typically used only for server-side?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this regarding the leading slash? I’ll change the code to favor source paths starting with a leading slash per the other comment.

Regarding join, we’re currently using it both for file system paths and for URLs. That’s a little messy since the operating system isn’t guaranteed to use a forward slash for a path delimiter, but it’ll be quite a bit of work to disentangle these two concepts if we want to… and I think even windows uses a forward slash now?

src/render.ts Outdated Show resolved Hide resolved
@mbostock mbostock requested a review from cinxmo November 9, 2023 03:15
@mbostock
Copy link
Member Author

mbostock commented Nov 9, 2023

Thanks for catching these bugs Cindy! I feel more confident that it is correct now. I want to followup with more tests after this (for example, for local fetch and promoting linked stylesheets to file attachments).

@cinxmo cinxmo mentioned this pull request Nov 9, 2023
6 tasks
@mbostock mbostock merged commit 3295682 into main Nov 9, 2023
1 check passed
@mbostock mbostock deleted the mbostock/fix-file-path branch November 9, 2023 15:06
This was referenced Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants