path: Make os an option argument

[nayeemrmn]

The export structure of the path module looks like this:

export const posix = {
  basename,
  dirname,
  extname,
  ...
};

export const win32 = {
  basename,
  dirname,
  extname,
  ...
};

export {
  basename,
  dirname, 
  extname,
  ...,
  globToRegExp,
  normalizeGlob,
  SEP,
  SEP_PATTERN,
  ...
};

This seems archaic and overly bound to where we vendored the implementation. It forces us to put group every function by OS in just path/posix.ts and path/win32.ts, rather than having modules like path/join.ts, path/file_url.ts, etc. It's hard to maintain and the test modules don't match it.

We should instead export one set of functions which each have an options interface containing the OS. They all have few enough positional arguments. This is modelled already by path/glob.ts, which gets to have its own module, containing:

export interface GlobOptions {
  ...,
  /** Operating system. Defaults to the native OS. */
  os?: typeof Deno.build.os;
}

These functions can branch by OS individually.

The current structure allows importing one namespace of functions for another OS, but this is niche enough that users should make such an abstraction themselves if they need it (in reality they can just import it from the node/ polyfill).

Ref https://github.com/denoland/deno/discussions/8405.

[lowlighter]

I can take a look at this later if no one else is working on it 👍🏼

I can see 2 ways to do it though, not sure which one is best:

1. Creating a new single file which abstract each function with the additional os option

//paths.ts
import * as _win32 from "./win32.ts";
import * as _posix from "./posix.ts";

export function basename(..., {os = Deno.build.os}:{os?:typeof Deno.build.os}) {
    return (os === "windows") ? _win32.basename(...arguments) : posix.basename(...arguments)
}

And we export * from "./path.ts" in mod.ts

2. Splitting win32.ts and posix.ts and reorder each function into a single file (like for test files)

//basename.ts
export function basename(..., {os = Deno.build.os}:{os?:typeof Deno.build.os}) {
    return (os === "windows") ? win32Basename(...arguments) : posixBasename(...arguments)
}

function _win32Basename(...) { /* old basename function from win32.ts */ }
function _posixBasename(...) { /* old basename function from posix.ts */ }

//other files for each path function which has a named file test

This breaks retro-compatibility with people importing directly from win32.ts or posix.ts instead of mod.ts

[bartlomieju]

@lowlighter are you still interested in working on this?

[lowlighter]

@bartlomieju I can work on it, just need to know which approach I should take

Personally I'm more for the 2nd one to be consistent with other std sections, but it breaks retro-compatibility. Or else I guess I could turn win32.ts and posix.ts into "virtual files" which re-exports by os again each function

[bartlomieju]

I don't have a strong opinion regarding this, @nayeemrmn what do you think?

[nayeemrmn]

Assuming breakages are okay, my vision was just removing win32.ts and posix.ts, shoving them all into mod.ts and giving each function a branch for os. We can split them further with demand down the line. But preserve the ones which are already split out, like common.ts, glob.ts and separator.ts.

// mod.ts
export * from "./common.ts";
export * from "./glob.ts";
export * from "./separator.ts";

export function basename(..., {os = Deno.build.os}:{os?:typeof Deno.build.os}) {
  if (Deno.build.os == "windows") {
    // windows implementation...
  } else {
    // unix implementation...
  }
}

export function dirname(..., {os = Deno.build.os}:{os?:typeof Deno.build.os}) {
  if (Deno.build.os == "windows") {
    // windows implementation...
  } else {
    // unix implementation...
  }
}

The reason I don't want to split the modules up front (even though that's one of the justifications of this design) is that the structure of those files could be in question whereas this is a forward-compatible option that could be pushed through slightly faster. Plus, I'm still interested in renaming the functions based on Rust stdlib and the file names would be based on those.

[lowlighter]

@nayeemrmn Ok so basically it's just merging win32 and posix to provide an abstraction to ease a future splitting?

Shoving them all in mod.ts seems strange, maybe it's just a misconception of mine, but I always thought that mod.ts were intended to be quite short. Should I go with a _path.ts instead, or do I just use the mod.ts directly ?

[nayeemrmn]

maybe it's just a misconception of mine, but I always thought that mod.ts were intended to be quite short. Should I go with a _path.ts instead, or do I just use the mod.ts directly ?

Having mod.ts import _path.ts which contains everything would be the exact same thing to the user as what I proposed, but redundant. Code splitting out of mod.ts only matters if those deps are public, which can be done later.

[lowlighter]

Seems that they are already tests files splitted for each function (basename_test.ts, dirname_test.ts, extname_test.ts, from_file_url_test.ts, isabsolute_test.ts, join_test.ts, parse_format_test.ts, relative_test.ts, resolve_test.ts, to_file_url_test.ts) so wouldn't it feel more natural to split everything directly following tests's current structure (and eventually rename them later) ? Or else I can put everything in the mod.ts like said above

Also I noticed that some functions are actually quite similar, should I merge their implementations to minimize code ?
For example, extname is actually the same for both posix and windows, except that the latter have special processing for drive letter, and there is also a minor difference for path seperator but 90% is the same. I won't do any refactor though, it'll just be factorization when it's adequate

[lino-levan]

I'll open a PR tomorrow to tackle this. This needs to be done before 1.0 imo.

[lino-levan]

We need to discuss requirements for std/path. What are the use-cases we envision for this module? Essentially, it gets broken down into these decisions (which do overlap a little bit):

• Do we want an option for specifying the os?
• Do we want to expose a posixX() and a windowsX() version of each X()?
• Do we keep posix.ts and win32.ts?
• Do we care about code duplication?
• How do we want APIs with ...args to function? Should those just accept an array instead?

These questions can only really be answered with concrete, supported use cases imo. I'm not really sure how to go about answering these.

[jsejcksn]

• How do we want APIs with ...args to function? Should those just accept an array instead?

I think that (generally for all of std), when accepting a collection, we should aim for the type Iterable<T> instead of an array of T unless there's something special about the implementation that actually requires an array. Forcing consumers to provide an array is arbitrarily restrictive in many cases, since there are plenty of other non-array, iterable structures.

Re: rest params vs a single iterable argument: I think there's going to be a tradeoff in ergonomics depending on usage patterns. In the examples below, I expect the second API to cause more developer surprises since they need to provide their own iterable container for a single argument:

// Example: Getting an absolute path from a relative one

// Current API: accepts rest params
path.resolve("relative_file.ext"); // "/Users/deno/relative_file.ext"

// API under discussion: accepts a single iterable param

// Intended usage:
path.resolve(["relative_file.ext"]); // "/Users/deno/relative_file.ext"

// ⚠️ Not a type error, but will iterate UTF-16 substrings,
// which is almost certainly not what the developer intended,
// and a potentially very difficult to catch bug:
path.resolve("relative_file.ext"); // "/Users/deno/r/e/l/a/t/i/v/e/_/f/i/l/e/./e/x/t"

If developer usage indicates that consumers are overwhelmingly likely to already have and use their own iterables, then the single-argument API would be more ergonomic than the current rest parameter API.

[jsejcksn]

From the initial post:

The current structure allows importing one namespace of functions for another OS, but this is niche enough that users should make such an abstraction themselves if they need it (in reality they can just import it from the node/ polyfill).

I am surprised to see this comment, especially this part:

this is niche enough that users should make such an abstraction themselves

IMO it's important to quantify (or at least qualify) descriptions like "niche" when considering addition or removal of APIs.

From my view, std should provide two things:

1. Utility primitives

   Alone, these don't really solve end-use cases, but are absolutely essential because they can be composed to solve all kinds of problems, especially niche ones that would be non-trivial to implement entirely from scratch in a considered way.

2. Utilities that address "common end-use cases"

   These are essentially multiple primitives from category 1, pre-glued together to create an API that ergonomically addresses a commonly-encountered task (e.g. loading environment variables from a file).

When I'm solving niche problems or implementing a new library, I find the category 1 parts to be indispensable. However, I find value much more often in the category 2 utilities (such as the ability to manipulate paths for a platform other than the one I'm currently using to run the code.)

[nayeemrmn]

However, I find value much more often in the category 2 utilities (such as the ability to manipulates paths for a platform other than the one I'm currently using to run the code.)

The use case I meant is the ability to pass around a path namespace belonging to whichever platform module to higher order functions. You would have the ability to manipulate cross-platform paths either way, with less brevity but more explicitness.

[lino-levan]

I believe that with #3649 landing, this issue isn't really relevant anymore. Thoughts @kt3k?

[kt3k]

@lino-levan I agree. Closing

Note: We didn't implement this (especially os argument part of the proposal). Instead, we decided to split submodules of std/path, path/windows and path/posix, to support os specific APIs (the same pattern is applied to glob APIs, which were previously using os-argument pattern. So now the all path APIs follow the same pattern). See the discussion in #3600 for more details.