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

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

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7221d92
typescript level conversion
mythmon Feb 21, 2024
8ad8455
remove only
mythmon Feb 21, 2024
9025a89
fix create tests
mythmon Feb 21, 2024
58705a3
optionally split build tests by platform
mythmon Feb 21, 2024
ffd4cd2
Merge remote-tracking branch 'origin/main' into mythmon/240216/brande…
mythmon Feb 21, 2024
f4699e5
tests passing locally
mythmon Feb 22, 2024
dcb4866
windows ci?
mythmon Feb 22, 2024
b3065f2
unskip build tests
mythmon Feb 22, 2024
254355d
add some ci debugging options
mythmon Feb 22, 2024
12118dc
more ci debugging
mythmon Feb 22, 2024
034bb47
normalize line endings in build tests
mythmon Feb 22, 2024
a3c73cf
change "simple" test data loader to js for windows compat
mythmon Feb 22, 2024
6b7c8c3
test matrix exclude
mythmon Feb 22, 2024
1b5aea5
add matrix parameters to artifact output
mythmon Feb 22, 2024
ca96b12
matrix exclude goes in matrix key
mythmon Feb 22, 2024
845e50a
fix paths in dynamic archive tests
mythmon Feb 22, 2024
12f91f9
add .gitattributes to enforce consistent line endings for test inputs
mythmon Feb 22, 2024
bcd8185
fix loader path better
mythmon Feb 22, 2024
aa162a0
always use posix-line endings
mythmon Feb 22, 2024
420c1f4
normalize all the line endings
mythmon Feb 22, 2024
1b6fdbc
debugging line endings
mythmon Feb 22, 2024
b7064b0
tsc
mythmon Feb 22, 2024
196c962
Revert "debugging line endings"
mythmon Feb 22, 2024
06801cb
ci test: change binary name?
mythmon Feb 22, 2024
2ecad19
better handle paths in create
mythmon Feb 22, 2024
dd7dc15
cleanup
mythmon Feb 22, 2024
a6a0a4a
fix expected output
mythmon Feb 22, 2024
9ddefa4
Remove excessive slashes in file:////
mythmon Feb 22, 2024
518f5bf
don't abbreviate UrlPath
mythmon Feb 22, 2024
6786e32
Merge branch 'main' into mythmon/240216/branded-paths
mythmon Feb 23, 2024
464ca6c
Fix path logic in preview's hello
mythmon Feb 23, 2024
80f4b90
lint
mythmon Feb 23, 2024
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
6 changes: 4 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@ jobs:
test:
strategy:
matrix:
version: [20, 21]
runs-on: ubuntu-latest
version: [20] # temporary remove 21 for testing
os: [ubuntu-latest, windows-latest]
fail-fast: false # temporary for testing
runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
Expand Down
12 changes: 8 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@
"observable": "bin/observable-init.js"
},
"scripts": {
"dev": "rm -f docs/themes.md docs/theme/*.md && (tsx watch docs/theme/generate-themes.ts & tsx watch --no-warnings=ExperimentalWarning ./bin/observable.ts preview --no-open)",
"build": "yarn rebuild-themes && rm -rf dist && tsx --no-warnings=ExperimentalWarning ./bin/observable.ts build",
"dev": "rimraf --glob docs/themes.md docs/theme/*.md && (tsx watch docs/theme/generate-themes.ts & tsx watch --no-warnings=ExperimentalWarning ./bin/observable.ts preview --no-open)",
"build": "yarn rebuild-themes && rimraf dist && tsx --no-warnings=ExperimentalWarning ./bin/observable.ts build",
"deploy": "yarn rebuild-themes && tsx --no-warnings=ExperimentalWarning ./bin/observable.ts deploy",
"rebuild-themes": "rm -f docs/themes.md docs/theme/*.md && tsx docs/theme/generate-themes.ts",
"rebuild-themes": "rimraf --glob docs/themes.md docs/theme/*.md && tsx docs/theme/generate-themes.ts",
"test": "yarn test:mocha && yarn test:tsc && yarn test:lint && yarn test:prettier",
"test:coverage": "c8 yarn test:mocha",
"test:mocha": "rm -rf test/.observablehq/cache test/input/build/*/.observablehq/cache && OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles tsx --no-warnings=ExperimentalWarning ./node_modules/.bin/mocha 'test/**/*-test.*'",
"test:mocha": "rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles tsx --no-warnings=ExperimentalWarning ./node_modules/mocha/bin/mocha.js 'test/**/*-test.*'",
"test:mocha-debug": "rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles tsx --no-warnings=ExperimentalWarning --inspect-brk ./node_modules/mocha/bin/mocha.js --timeout 3600000 'test/**/*-test.*'",
"test:lint": "eslint src test --max-warnings=0",
"test:prettier": "prettier --check src test",
"test:tsc": "tsc --noEmit",
Expand All @@ -53,6 +54,7 @@
"acorn": "^8.11.2",
"acorn-walk": "^8.3.0",
"ci-info": "^4.0.0",
"cross-spawn": "^7.0.3",
"esbuild": "^0.19.8",
"fast-array-diff": "^1.1.0",
"fast-deep-equal": "^3.1.3",
Expand Down Expand Up @@ -92,6 +94,7 @@
"c8": "^8.0.1",
"chai": "^4.3.10",
"chai-http": "^4.4.0",
"cross-env": "^7.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
"cross-env": "^7.0.3",

"d3-array": "^3.2.4",
"d3-dsv": "^3.0.1",
"eslint": "^8.50.0",
Expand All @@ -100,6 +103,7 @@
"eslint-plugin-import": "^2.29.0",
"mocha": "^10.2.0",
"prettier": "^3.0.3 <3.1",
"rimraf": "^5.0.5",
"typescript": "^5.2.2",
"undici": "^5.27.2"
},
Expand Down
100 changes: 100 additions & 0 deletions src/brandedFs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import fs, {type MakeDirectoryOptions, type Stats, type WatchListener, type WriteFileOptions} from "node:fs";
import fsp, {type FileHandle} from "node:fs/promises";
import {FilePath, unFilePath} from "./brandedPath.js";

export const constants = fsp.constants;

export function access(path: FilePath, mode?: number): Promise<void> {
return fsp.access(unFilePath(path), mode);
}

export function accessSync(path: FilePath, mode?: number): void {
return fs.accessSync(unFilePath(path), mode);
}

export function copyFile(source: FilePath, destination: FilePath, flags?: number): Promise<void> {
return fsp.copyFile(unFilePath(source), unFilePath(destination), flags);
}

export function readFile(path: FilePath, encoding?: undefined): Promise<Buffer>;
export function readFile(path: FilePath, encoding: BufferEncoding): Promise<string>;
export function readFile(path: FilePath, encoding?: BufferEncoding | undefined): Promise<string | Buffer> {
return fsp.readFile(unFilePath(path), encoding);
}

export function readFileSync(path: FilePath, encoding: BufferEncoding): string {
return fs.readFileSync(unFilePath(path), encoding);
}

export function writeFile(path: FilePath, data: string | Buffer, options?: WriteFileOptions): Promise<void> {
return fsp.writeFile(unFilePath(path), data, options);
}

export function writeFileSync(path: FilePath, data: string | Buffer, options?: WriteFileOptions): void {
return fs.writeFileSync(unFilePath(path), data, options);
}

export async function mkdir(path: FilePath, options?: MakeDirectoryOptions): Promise<FilePath | undefined> {
const rv = await fsp.mkdir(unFilePath(path), options);
return rv ? FilePath(rv) : undefined;
}

export function readdir(path: FilePath): Promise<string[]> {
return fsp.readdir(unFilePath(path));
}

export function stat(path: FilePath): Promise<Stats> {
return fsp.stat(unFilePath(path));
}

export function open(path: FilePath, flags: string | number, mode?: number): Promise<FileHandle> {
return fsp.open(unFilePath(path), flags, mode);
}

export function rename(oldPath: FilePath, newPath: FilePath): Promise<void> {
return fsp.rename(unFilePath(oldPath), unFilePath(newPath));
}

export function renameSync(oldPath: FilePath, newPath: FilePath): void {
return fs.renameSync(unFilePath(oldPath), unFilePath(newPath));
}

export function unlink(path: FilePath): Promise<void> {
return fsp.unlink(unFilePath(path));
}

export function unlinkSync(path: FilePath): void {
return fs.unlinkSync(unFilePath(path));
}

export function existsSync(path: FilePath): boolean {
return fs.existsSync(unFilePath(path));
}

export function readdirSync(path: FilePath): FilePath[] {
return fs.readdirSync(unFilePath(path)) as unknown as FilePath[];
}

export function statSync(path: FilePath): Stats {
return fs.statSync(unFilePath(path));
}

export function watch(path: FilePath, listener?: WatchListener<string>): fs.FSWatcher {
return fs.watch(unFilePath(path), listener);
}

export function utimes(path: FilePath, atime: Date, mtime: Date): Promise<void> {
return fsp.utimes(unFilePath(path), atime, mtime);
}

export function utimesSync(path: FilePath, atime: Date, mtime: Date): void {
return fs.utimesSync(unFilePath(path), atime, mtime);
}

export function createReadStream(path: FilePath): fs.ReadStream {
return fs.createReadStream(unFilePath(path));
}

export function rm(path: FilePath, options?: {force?: boolean; recursive?: boolean}): Promise<void> {
return fsp.rm(unFilePath(path), options);
}
101 changes: 101 additions & 0 deletions src/brandedPath.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import osPath from "node:path";
import posixPath from "node:path/posix";

// including "unknown &" here improves type error message
type BrandedString<T> = unknown &
Omit<string, "replace" | "slice"> & {__type: T} & {
replace: (search: string | RegExp, replace: string) => BrandedString<T>;
slice: (start?: number, end?: number) => BrandedString<T>;
};

export type FilePath = BrandedString<"FilePath">;
export type UrlPath = BrandedString<"UrlPath">;

export function FilePath(path: string | FilePath): FilePath {
if (osPath.sep === "\\") {
path = path.replaceAll("/", osPath.sep);
} else if (osPath.sep === "/") {
path = path.replaceAll("\\", osPath.sep);
}
return path as unknown as FilePath;
}

export function UrlPath(path: string | UrlPath): UrlPath {
path = path.replaceAll("\\", posixPath.sep);
return path as unknown as UrlPath;
}

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;
}

Comment on lines +28 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

export function filePathToUrlPath(path: FilePath): UrlPath {
if (osPath.sep === "/") return path as unknown as UrlPath;
return urlJoin(...(path.split(osPath.sep) as string[]));
}

export function urlPathToFilePath(path: UrlPath): FilePath {
if (osPath.sep === "/") return path as unknown as FilePath;
return fileJoin(...(path.split(posixPath.sep) as string[]));
}

// Implemenations of node:path functions:

export function urlJoin(...paths: (string | UrlPath)[]): UrlPath {
return posixPath.join(...(paths as string[])) as unknown as UrlPath;
}

export function fileJoin(...paths: (string | FilePath)[]): FilePath {
return osPath.join(...(paths as string[])) as unknown as FilePath;
}

export function fileRelative(from: string | FilePath, to: string | FilePath): FilePath {
return FilePath(osPath.relative(unFilePath(from), unFilePath(to)));
}

export function fileDirname(path: string | FilePath): FilePath {
return FilePath(osPath.dirname(unFilePath(path)));
}

export function urlDirname(path: string | UrlPath): UrlPath {
return UrlPath(posixPath.dirname(unUrlPath(path)));
}

export function fileNormalize(path: string | FilePath): FilePath {
return FilePath(osPath.normalize(unFilePath(path)));
}

export function urlNormalize(path: string | UrlPath): UrlPath {
return UrlPath(osPath.normalize(unUrlPath(path)));
}

export function fileBasename(path: string | FilePath, suffix?: string): string {
return osPath.basename(unFilePath(path), suffix);
}

export function urlBasename(path: string | UrlPath, suffix?: string): string {
return osPath.basename(unUrlPath(path), suffix);
}

export function fileExtname(path: string | FilePath | string): string {
return osPath.extname(unFilePath(path));
}

export function urlExtname(path: string | UrlPath | string): string {
return osPath.extname(unUrlPath(path));
}

export function fileResolve(...paths: (string | FilePath)[]): FilePath {
return FilePath(osPath.resolve(...(paths as string[])));
}

export function urlResolve(...paths: (string | UrlPath)[]): UrlPath {
return UrlPath(osPath.resolve(...(paths as string[])));
}

export const fileSep = osPath.sep as unknown as FilePath;
export const urlSep = posixPath.sep as unknown as UrlPath;
Loading
Loading