Distinguish between URL paths and file paths at the type level #881

mythmon · 2024-02-20T16:29:43Z

This is an attempt to improved Windows support. The idea here is that we can use TypeScript to tag some strings as being either FilePaths or UrlPaths. From there we can make a series of helpers around the Node filesystem and path functions that are stricter about which one they take.

I'm putting this up as a draft now to get some early feedback about this approach. I think there is some inherent boilerplate here to tag strings as the two types. I think there may also be some boilerplate in this PR that we could get rid of. Any comments or suggestions about how we could improve this are welcome!

As of now, I have all the tests passing on Windows locally.

Besides the path annotations, I've introduced some libraries to ease in cross-platform compatibility:

cross-env - Handles setting environment variables inline in package.json scripts
rimraf - A cross platform rm-like tool
cross-spawn - For executing files more easily in a cross platform way.

I'm not sure how we can get .sh data loaders working, so for now I've simply disabled those tests on Windows. I think we can save that for another PR.

In order for our snapshot tests to work, I added a .gitattributes file that insists on posix-style line endings everywhere. I based that off of these GitHub docs and this blogpost.

To Test

Check out this branch
Run yarn install to update packages
Run yarn observable create to create a new project
cd into the project directory
Edit the project to use the branch instead of the npm published version of Framework:
- yarn remove @observablehq/framework
- yarn add link:..
Use Framework with commands like yarn dev, yarn build, yarn observable login and yarn deploy.

…d-paths

Fil

two small questions for now

Fil · 2024-02-22T06:27:05Z

src/brandedPath.ts

+export function unFilePath(path: FilePath | string): string {
+  return path as unknown as string;
+}
+
+export function unUrlPath(path: UrlPath | string): string {
+  return path as unknown as string;
+}
+


these no-op functions are here only for the sake of typing (or, untyping), there should be a better way to do this with typescript only?

One thing that I was disappointed about was that I couldn't figure out how to safely tell TypeScript that UrlPath and FilePath were actually just type safe strings. If we remove the overrides of replace and similar in the definitions of the path types, it can understand it, but then we erase types when working on the path.

I think if we wanted to eliminate these functions, we'd have to inline the as unknown as string text everywhere we call this function. I chose not to do that because I treat as-casts as dangerous code. I'd prefer to isolate it to as few places as possible. Then those isolated places can be thoroughly vetted, either through tests or through reading the code. In this case the cast is trivial to verify, so just having this function is enough.

I agree though that I would prefer some better way to do this, like maybe a "type level function". I don't know of one though.

All of the wrapping worries me, but I can't express why. It feels excessive or invasive maybe? I'm definitely wishing we didn't have to do it.

This pattern, of having a new type that is just an existing type with a new name is something that I've used in other languages. In those languages there are often nice features built into the language that help make this easier, like implicit conversion, operator overloading, and monad-style-map. TypeScript doesn't really give us any of those things, so it is all a bit clunky.

I could imagine taking out most of these wrappers now (except for the places where we need slash conversion). I don't know how we would prevent new changes to the code base from getting it wrong in the future. Maybe just test coverage and Windows in CI? That feels much less robust than TypeScript to me.

src/config.ts

Fil

If I understand correctly, this PR includes 3 changes that are almost independent:

generalizes some system call (rimraf and cross-env)
types all paths as either filePath or urlPath
carries path.sep into the internal representation of file paths

(It also adds platform-specific tests—in CI, and in unit tests.)

Generalizations/abstractions seem like the right thing to do. Adding platform-specific tests is obviously needed.

For step 2, I think it's great that you're typing these paths as "filesystem path" or "url path", as it helps bring clarity to the code, where it's easy otherwise to get a bit confused between what the paths are for. It certainly helps with step 3. However in my mind it's almost an orthogonal concern.

Step 3 feels overwhelming. Our app code now sees different strings depending on the OS it runs on, and has to reason about them differently. On windows, file paths will be a string docs\sub\file.md, and on linux an macos it will be docs/sub/file.md. This representation is carried throughout, meaning that at every step we need to convert between filepath and urlpath (which always use /). In a sense, now every line of code needs to know about windows.

Could we consider an approach where we would normalize the internal representation of all paths to forward slashes, and adapt/convert only when talking to the filesystem? Basically an abstraction layer on top of fs that would use /-delimited paths as inputs and outputs, restricting the use of \ to the underlying OS calls.

An analogy that comes to mind is to use an ORM if we want to talk to different database flavors. If that is possible, it means we could do a much smaller step 3.

More importantly, when some logic needs to reason about a path, say "if this file is foo/bar, do something", it can be written directly rather than having to call conversion functions. For a concrete example: when we create the search index (in search.ts), we attribute an id to each page, which corresponds to its relative path to the docs root. That id is also used as a link[href] when we present the results in the browser. I’d rather keep this logic insulated from having to care about windows-style paths.

If that approach proves possible, we could even skip step 2 altogether for now (and do it separately later, independently of windows support).

We could also land step 1 separately.

Finally, abstracting fs might also help with unicode file names (for international text or even emoji #783).

mythmon · 2024-02-22T16:49:51Z

Thanks for the comments, Fil.

Your first two summary points I agree with. I'm not sure if it makes sense to split 1 out, since the project is still quite broken even with those changes in place.

I don't quite agree about the third though. path.sep was already in the internal representation of these paths when running on Windows. However, it was there in an uncontrolled, chaotic way. When working through the test cases, I occasionally found paths like foo/bar\baz where different slashes had been mixed within one path. What this PR does is make the existing code explicitly handle that.

I like the idea of using internally normalized paths.

For me the biggest hurdle is that it would require writing wrappers around all of node:path and node:fs. However, as a part of this implementation I ended up writing writing these kind of wrappers anyways in the form of the branded path and fs modules.

The other concern I have is about the appropriateness of abstraction. Framework has to deal with both files and HTTP very directly. There are a few places where we need to understand the differences between win32 and posix paths. I'm not sure if that is fundamental complexity, or if we would prefer to work in an abstracted space where we don't have to worry about it.

Given that, I plan to continue with this "slash-aware" version, and get it passing in CI. After I'm done with that, I can explore the idea of internally normalized paths, if someone else doesn't beat me to it.

Regarding the FilePath/UrlPath annotations: Given that the work is fundamentally done in that area, I think it would make sense to keep it. Even when we aren't mixing slashes, it made the intent of some of the code clearer for me.

Fil · 2024-02-22T17:21:32Z

path.sep was already in the internal representation

Claro. (I did not mean to imply that it was working before.) My question is whether we can control it at the file system interface, and continue ignoring it in the app code (but, confident in that it is safe and controlled).

mythmon · 2024-02-22T18:39:07Z

test/dataloaders-test.ts

-    const time = Date.UTC(2023, 11, 1) / 1000;
-    await utimes(loader.path, atime, time);
-    // remove the cache set by another test (unless we it.only this test).
-    try {
-      await unlink("test/.observablehq/cache/dataloaders/data1.txt");
-    } catch {
-      // ignore;
    }
-    // populate the cache (missing)
-    await loader.load(outputEffects);
-    // run again (fresh)
-    await loader.load(outputEffects);
-    // touch the loader
-    await utimes(loader.path, atime, Date.now() + 100);
-    // run it with useStale=true (using stale)
-    const loader2 = Loader.find("test", "dataloaders/data1.txt", {useStale: true})!;
-    await loader2.load(outputEffects);
-    // run it with useStale=false (stale)
-    await loader.load(outputEffects);
-    // revert the loader to its original mtime
-    await utimes(loader.path, atime, mtime);
-    assert.deepStrictEqual(
-      // eslint-disable-next-line no-control-regex
-      out.map((l) => l.replaceAll(/\x1b\[[0-9]+m/g, "")),
-      [
-        "load test/dataloaders/data1.txt.js → ",
-        "[missing] ",
-        "load test/dataloaders/data1.txt.js → ",
-        "[fresh] ",
-        "load test/dataloaders/data1.txt.js → ",
-        "[using stale] ",
-        "load test/dataloaders/data1.txt.js → ",
-        "[stale] "
-      ]
-    );
-  });
+  };
+  const loader = Loader.find(FilePath("test"), FilePath("dataloaders/data1.txt"))!;
+  // save the loader times.
+  const {atime, mtime} = await stat(loader.path);
+  // set the loader mtime to Dec. 1st, 2023.
+  const time = new Date("2023-11-01");


I changed this from "seconds since epoch" to a Date because non-date values throw an error on Windows: nodejs/node#5561 (comment)

This reverts commit 1b6fdbc.

mythmon · 2024-02-22T21:28:06Z

examples/eia/docs/data/eia-bia-reference.csv

I converted this file to use LF endings instead of CRLF. If modifying this file isn't ok, I could add an exception to .gitattributes for it.

mythmon · 2024-02-23T00:03:47Z

With this branch, I was able to create, preview, build, login, and deploy in Windows. It's a big change, but I think most of the changes only affect their immediate area. It works for me, and it works on CI, though I would certainly appreciate more testing on this, both on Windows machines as well as Non-Windows.

I'm going to start another PR based on this to explore Fil's idea of using forward slashes everywhere internally, and only converting to OS-correct paths at the last moment.

mythmon · 2024-02-22T23:48:29Z

bin/observable-init.js

We can sort of use shebangs on Windows, but we can't do complicated things like pass arguments with -S. I think that this new version is equivalent to what we had before.

mythmon · 2024-02-22T23:49:17Z

examples/eia/docs/data/eia-bia-reference.csv

As a part of my .gitattributes changes, I converted the line endings of this file (and the next one) from \r\n to \n. I can put them back and update .gitattributes if it matters.

mythmon · 2024-02-22T23:53:26Z

src/commandInstruction.ts

-  if (!color) color = (s) => s;
+  if (!color) color = (s) => `${s}`;


This lets us directly print branded strings, since they implement { toString(): string }.

mythmon · 2024-02-22T23:59:54Z

test/observableApiConfig-test.ts

  cwd() {
-    return join("/", "opt", "projects", "acme-bi");
+    // it is an important detail that this is not inside the home dir
+    return isWindows ? fileJoin("D:", "Projects", "acme-bi") : fileJoin("/", "opt", "projects", "acme-bi");
  }


@Fil this is one thing I'm not sure how to handle in a "normalized internal paths" approach. Would we use hybrid Windows/Posix paths, like C:/Projects/acme-bi?

mythmon · 2024-02-23T00:11:17Z

src/brandedPath.ts

+export function unFilePath(path: FilePath | string): string {
+  return path as unknown as string;
+}
+
+export function unUrlPath(path: UrlPath | string): string {
+  return path as unknown as string;
+}
+


This pattern, of having a new type that is just an existing type with a new name is something that I've used in other languages. In those languages there are often nice features built into the language that help make this easier, like implicit conversion, operator overloading, and monad-style-map. TypeScript doesn't really give us any of those things, so it is all a bit clunky.

I could imagine taking out most of these wrappers now (except for the places where we need slash conversion). I don't know how we would prevent new changes to the code base from getting it wrong in the future. Maybe just test coverage and Windows in CI? That feels much less robust than TypeScript to me.

GordonSmith · 2024-02-24T08:33:16Z

Just a lurker here - and don't want to get into the whole functional v OOP debate - but wouldn't this change be a lot cleaner with a FilePath/URLPath Classes (or more use of the Node URL Class)?

mythmon · 2024-02-24T20:37:01Z

@GordonSmith thanks for the comments. I'm not sure if a class based approach would help. The original intent was to do this entirely at the TypeScript, except where conversion was needed. It would be nice if this had as little runtime overhead as possible. I'm not sure how well that worked out though. I think that a class would only be a change of "spelling" without really changing the overall cleanliness.

I'm working on an alternative approach that fill Fil recommended that doesn't use any wrapping at all. I think it is promising, but we'll see once I get it working.

As for the URL class, as far as I know that requires full, absolute URLs, including hostname. I don't think that would work well here.

mbostock · 2024-03-01T17:35:05Z

.gitattributes

I think we should disable autocrlf.

* text=false

mbostock · 2024-03-01T17:36:08Z

bin/observable-init.js

I want to remove our runtime dependency on tsx. #850 Also, we’ll need the same fix in the @observablehq/create repo.

mbostock · 2024-03-01T17:36:29Z

examples/eia/docs/data/eia-bia-reference.csv

Let’s leave these as-is to minimize this PR.

mbostock · 2024-03-01T17:37:04Z

package.json

@@ -92,6 +94,7 @@
    "c8": "^8.0.1",
    "chai": "^4.3.10",
    "chai-http": "^4.4.0",
+    "cross-env": "^7.0.3",


If this is in dependencies already, we shouldn’t also need it in devDependencies.

Suggested change

"cross-env": "^7.0.3",

mythmon · 2024-03-02T01:16:51Z

Closing in favor of #944.

typescript level conversion

7221d92

mythmon force-pushed the mythmon/240216/branded-paths branch from 82b8c8e to 7221d92 Compare February 21, 2024 23:32

mythmon added 6 commits February 21, 2024 15:34

remove only

8ad8455

fix create tests

9025a89

optionally split build tests by platform

58705a3

Merge remote-tracking branch 'origin/main' into mythmon/240216/brande…

ffd4cd2

…d-paths

tests passing locally

f4699e5

windows ci?

dcb4866

Fil reviewed Feb 22, 2024

View reviewed changes

mythmon added 3 commits February 22, 2024 09:59

unskip build tests

b3065f2

add some ci debugging options

254355d

more ci debugging

12118dc

mythmon commented Feb 22, 2024

View reviewed changes

mythmon added 14 commits February 22, 2024 11:00

normalize line endings in build tests

034bb47

change "simple" test data loader to js for windows compat

a3c73cf

test matrix exclude

6b7c8c3

add matrix parameters to artifact output

1b5aea5

matrix exclude goes in matrix key

ca96b12

fix paths in dynamic archive tests

845e50a

add .gitattributes to enforce consistent line endings for test inputs

12f91f9

fix loader path better

bcd8185

always use posix-line endings

aa162a0

normalize all the line endings

420c1f4

debugging line endings

1b6fdbc

tsc

b7064b0

Revert "debugging line endings"

196c962

This reverts commit 1b6fdbc.

ci test: change binary name?

06801cb

mythmon commented Feb 22, 2024

View reviewed changes

mythmon added 6 commits February 22, 2024 15:19

better handle paths in create

2ecad19

cleanup

dd7dc15

fix expected output

a6a0a4a

Remove excessive slashes in file:////

9ddefa4

don't abbreviate UrlPath

518f5bf

Merge branch 'main' into mythmon/240216/branded-paths

6786e32

mythmon marked this pull request as ready for review February 23, 2024 00:00

mythmon commented Feb 23, 2024

View reviewed changes

Fix path logic in preview's hello

464ca6c

mythmon mentioned this pull request Feb 23, 2024

Support Windows #90

Closed

lint

80f4b90

mythmon mentioned this pull request Feb 27, 2024

Use posix-style paths internally, only convert for IO #922

Closed

mbostock reviewed Mar 1, 2024

View reviewed changes

.gitattributes

Copy link

Member

mbostock Mar 1, 2024

Choose a reason for hiding this comment

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

I think we should disable autocrlf.

* text=false

mbostock reviewed Mar 1, 2024

View reviewed changes

examples/eia/docs/data/eia-bia-reference.csv

Copy link

Member

mbostock Mar 1, 2024

Choose a reason for hiding this comment

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

Let’s leave these as-is to minimize this PR.

mbostock reviewed Mar 1, 2024

View reviewed changes

mythmon closed this Mar 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distinguish between URL paths and file paths at the type level #881

Distinguish between URL paths and file paths at the type level #881

mythmon commented Feb 20, 2024 •

edited

Loading

Fil left a comment

Fil Feb 22, 2024

mythmon Feb 22, 2024

visnup Feb 23, 2024

mythmon Feb 23, 2024

Fil left a comment

mythmon commented Feb 22, 2024

Fil commented Feb 22, 2024

mythmon Feb 22, 2024

mythmon Feb 22, 2024

mythmon commented Feb 23, 2024

mythmon Feb 22, 2024

mythmon Feb 22, 2024

mythmon Feb 22, 2024

mythmon Feb 22, 2024

mythmon Feb 23, 2024

GordonSmith commented Feb 24, 2024 •

edited

Loading

mythmon commented Feb 24, 2024

mbostock Mar 1, 2024

mbostock Mar 1, 2024

mbostock Mar 1, 2024

mbostock Mar 1, 2024

mythmon commented Mar 2, 2024

		if (!color) color = (s) => s;
		if (!color) color = (s) => `${s}`;

Distinguish between URL paths and file paths at the type level #881

Distinguish between URL paths and file paths at the type level #881

Conversation

mythmon commented Feb 20, 2024 • edited Loading

To Test

Fil 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

Fil left a comment

Choose a reason for hiding this comment

mythmon commented Feb 22, 2024

Fil commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythmon commented Feb 23, 2024

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

GordonSmith commented Feb 24, 2024 • edited Loading

mythmon commented Feb 24, 2024

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

mythmon commented Mar 2, 2024

mythmon commented Feb 20, 2024 •

edited

Loading

GordonSmith commented Feb 24, 2024 •

edited

Loading