Improve Path.parse / Path.format combo

[ivan-kleshnin]

We have Path.format / Path.parse functions.
They can be chained which is very convenient.

import Path from "path";

let path = "/foo/bar/bazz.js";
let pathP = Path.format(Path.parse(path));

Currently parse converts string to an object with such structure

{ root: '/',
  dir: '/Users/ivankleshnin/Projects/demo',
  base: 'config.yml',
  ext: '.yml',
  name: 'config' }

This object contains denormalized data between base, name and ext key values.
Now let's try to replace file extension.

import Path from "path";
import {assoc} from "ramda"

let path = "/Users/ivankleshnin/Projects/demo/config.js";
let pathP = assoc("ext", ".json", Path.parse(path));
/* {...
  base: 'config.js',  -- these two are 
  ext: '.json',       -- unsynced!
...} */
console.log(Path.format(pathP)); // extension weren't changed :(

The simplest task is going to be not so simple?!
Now if format took into consideration ext and name rather than base this could lead to an equal problem with changes to base key being ignored.

Can we get rid of this base key? It's always parsed.name + parsed.ext formula, not a big deal to make it manually. Example of hidden file parse: { base: '.gitignore', ext: '', name: '.gitignore' } - same rule apply.

We can probably also implement it in a backward-compatibile way,
keeping base but using JS getter / setter for it's evaluation.

[benjamingr]

So your suggestion is that base becomes a getter/setter?

That doesn't sound too bad but I wonder what the impact on backwards compatibility is

[ivan-kleshnin]

Examples:

// CURRENT BEHAVIOR 
let path0 = "/Users/ivankleshnin/Projects/demo/config.yml";

let parsed0 = Path.parse(path0);
console.log(Path.format(parsed0)); // /Users/ivankleshnin/Projects/demo/config.yml (+)

parsed0.name = "xxx";
console.log(Path.format(parsed0)); // /Users/ivankleshnin/Projects/demo/config.yml (-)

parsed0.ext = ".json";
console.log(Path.format(parsed0)); // /Users/ivankleshnin/Projects/demo/config.yml (-)

parsed0.base = "test.html";
console.log(Path.format(parsed0)); // /Users/ivankleshnin/Projects/demo/test.html (+)

// NEW BEHAVIOR 
let path1 = "/Users/ivankleshnin/Projects/demo/config.yml";

let parsed1 = newParse(path1);
console.log(Path.format(parsed1)); // /Users/ivankleshnin/Projects/demo/config.yml (+)

parsed1.name = "xxx";
console.log(Path.format(parsed1)); // /Users/ivankleshnin/Projects/demo/xxx.yml (+)

parsed1.ext = ".json";
console.log(Path.format(parsed1)); // /Users/ivankleshnin/Projects/demo/xxx.json (+)

parsed1.base = "test.html";
console.log(Path.format(parsed1)); // /Users/ivankleshnin/Projects/demo/test.html (+)

Proof implementation:

...

function newParse(pathString) {
  ...

  Object.defineProperty(parsed, "base", {
    enumerable: true,
    configurable: false,
    get: function () {
      return this.name + this.ext;
    },
    set: function (value) {
      if (value.startsWith(".") || !value.includes(".")) {
        this.name = value;
        this.ext = "";
      } else {
        let [name, ext] = value.split(".");
        this.name = name;
        this.ext = "." + ext;
      }
    }
  });

  return parsed;
}

Self-contained working gist

Should be fully backward compatible, unless I miss something.

To be precise: will break code which depends on base changes not propagated to name and ext i.e. on the "buggy" aspect of current behavior. Hard to imagine such code IMO.

Platform Requirements

(of possible feature implementation, not a provided gist)

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty#Configurable_attribute

Basically: IE 9+

Note: I'm not aware of platform support requirements of IO JS.

[nwoltman]

IMO this sort of functionality should be implemented in user-land (npm). The path.parse function is just a convenient way to split a path into its main components and path.format is mainly for completeness so we can return the path components to the original path string (see the original issue).

[benjamingr]

@nodejs/collaborators anyone wants to promote this or should we close the issue?

[sam-github]

The current behaviour is indeed bizarre, it one of the things I talk about in https://www.youtube.com/watch?v=jJaIwea8r2A

Its not only strange in and of itself, its also inconsistent with node's url.parse/url.format.

@nwoltman Is your suggestion to deprecate the path module and promote an npm module to take its place?

[refack]

I'm interested.

[nwoltman]

@sam-github In case it helps, the example you gave from your talk:

const path = require('path');
const bits = path.parse('some/dir/index.txt');
console.log(bits.base); // > index.txt
delete bits.base;
bits.ext = '.html';
console.log(path.format(bits));

does work like the url module now, so it outputs 'some/dir/index.html'. Also, path.format() is documented well enough now that people should know what to expect when using it.

@nwoltman Is your suggestion to deprecate the path module and promote an npm module to take its place?

I didn't mean to suggest to deprecate the path module. What I meant was that "extended" functionality (such as having getters/setters on the object returned by path.parse()) should be provided by an npm module. There's a similar situation with the querystring core module where there's an npm module called qs that provides more functionality than the core module.

[refack]

I have a faint memory that all the logic was in parse and the others just projected parts of the parse resault... Now I'm confused why is there quadruple duplication (win/posix × parse/specific)?

[Trott]

Is the consensus here that this should be a userland npm module? Or is this a bug in Node.js that should be fixed?

[TimothyGu]

I think the general consensus established in #12818 was that Object.defineProperty was too slow to be used per-run on the returned object, and defining the getter on the returned object's prototype would cause too many breakages due to Object.keys() no longer returning that property. While I agree that making base an accessor property would be a better API design, I think we are stuck with it unfortunately.

@tniessen @refack Thoughts?

[refack]

#12511 is half way there. It keeps blinking in and out of my focus... AFAICT it's a less complicated issue than Stats but a thorough breakage analysis will need to be done.

[refack]

[rant warning]
Now I remember what happened... scope-creep...

and it's really a pain developing on Windows doing a change-compile-test cycle takes ~30 minutes, which got me focusing more and more or solving that, and puff there goes productivity...