[partial work] path: improve parse => format combination #12511

refack · 2017-04-19T13:19:14Z

make path.parse return an object where base is a computed property

Ref: #1999

TODO:

Add test
Update doc with new capabilities

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

path

refack · 2017-04-19T13:20:20Z

~~multi-platform test: https://ci.nodejs.org/job/node-test-pull-request/7515/~~

benjamingr · 2017-04-19T13:28:35Z

Adding semver-major since this is a breaking API change. I'm +1 on the actual change.

Also pinging @nodejs/collaborators since this is an API change in case anyone has anything to add.

refack · 2017-04-19T13:37:48Z

I want to ask is it semver-major? the api change is that base moved from being a value property into a computed property?
(I lean semver-minor, if all tests pass, but no strong opinion)
on Second tought, it changes (probably undocumented) behaviour, so semver-major indeed.
As for the don't land that could be debated.

refack · 2017-04-19T13:41:54Z

multi-platform test: https://ci.nodejs.org/job/node-test-pull-request/7517/

jasnell · 2017-04-19T14:22:27Z

Well, semver-major automatically implies don't land on any of the current or LTS releases so there's no much to debate ;-)

benjamingr · 2017-04-19T14:37:12Z

@refack those don't-land-on are just to indicate that it's a backwards incompatible change so it should not be ported to older versions. It's a given given semver-major.

If you feel strongly about this not being semver-major I'm open to discussion, but I find it hard to justify a minor version breaking existing code - this is not just people relying on undocumented behavior, this is an API change in an API marked as Stability: 2 - Stable.

refack · 2017-04-19T15:24:33Z

Well, semver-major automatically implies don't land on any of the current or LTS releases so there's no much to debate ;-)

Yeah, Obv. I just had a convoluted thought...

@refack those don't-land-on are just to indicate that it's a backwards incompatible change so it should not be ported to older versions. It's a given given semver-major.

If you feel strongly about this not being semver-major I'm open to discussion, but I find it hard to justify a minor version breaking existing code - this is not just people relying on undocumented behavior, this is an API change in an API marked as Stability: 2 - Stable.

I don't feel strongly, just a little clarifying discussion to help me understand the common assumptions:

I totally agree that changing stable API is semver-major (to the point of being ridiculous assert.fail() accept a single argument or two arguments #12293 (comment))
Question: only thing changed is the "mechanics" of a property in the returned value in https://nodejs.org/api/path.html#path_path_parse_path. Is that even considered a change?
If all tests pass with no tests changes made by me, (like the proverbial tree in the woods) did anything change 🕴️ ?

refack · 2017-04-19T15:30:58Z

P.S. If we're talking breaking changes, how about me adding a .format() method to the returned type?
Also finding a way to make it compatible with url.format() so we get a file://.... output?

mscdex · 2017-04-19T16:51:22Z

lib/path.js

+      configurable: false,
+      get() { return this.name + this.ext; },
+      set(value) {
+        if (value.startsWith('.') || !value.includes('.')) {


Why do we need both of these? Can't we combine both using one call to value.indexOf()?:

var dotIdx = value.indexOf('.'); if (dotIdx <= 0) { // ...

actually this is wrong, in Windows .test.exe is considered and exe file

mscdex · 2017-04-19T16:52:42Z

lib/path.js

+      configurable: false,
+      get() { return this.name + this.ext; },
+      set(value) {
+        if (value.startsWith('.') || !value.includes('.')) {


mscdex · 2017-04-19T16:59:44Z

Also, be sure that the const or other changes do not cause any permanent deopts. You can use either #12456 or do it manually (e.g. after compiling: ./node --trace-opt --always-opt --trace-file-names test/parallel/test-path-parse-format.js | grep -i 'aborted\|disabled').

refack · 2017-04-19T17:57:03Z

Ohh, found the real API change (it was in path.format 🤦)

When providing properties to the `pathObject` remember that there are
combinations where one property has priority over another:

* `pathObject.root` is ignored if `pathObject.dir` is provided
* `pathObject.ext` and `pathObject.name` are ignored if `pathObject.base` exists
...

refack · 2017-04-19T18:06:39Z

No wait, I'm confused 😵 the above described behavior still stands, and this test passes:

    const output = path.parse(element);
    assert.strictEqual(typeof output.root, 'string');
    assert.strictEqual(typeof output.dir, 'string');
    assert.strictEqual(typeof output.base, 'string');
    assert.strictEqual(typeof output.ext, 'string');
    assert.strictEqual(typeof output.name, 'string');
    assert.strictEqual(path.format(output), element);

refack · 2017-04-19T19:04:57Z

Also, be sure that the const or other changes do not cause any permanent deopts.

No new deopts.

nwoltman · 2017-04-20T16:57:56Z

There should also be a similar getter/setter for dir (since root forms the first part of dir and changing dir should update root).

joyeecheung · 2017-04-21T06:26:44Z

lib/path.js

+        const li = value.lastIndexOf('.');
+        if (li > 0) {
+          this.ext = value.slice(li);
+          this.name = value.split(0, li);


isn't this supposed to be slice?

obv. 🤦
Need to writes tests, and finish this PR.

thefourtheye · 2017-04-21T15:13:27Z

lib/path.js

@@ -1136,6 +1129,22 @@ const win32 = {
    else if (isAbsolute)
      ret.dir = path.slice(0, rootEnd);

+    Object.defineProperty(ret, 'base', {


Same code repeated. Can this be refactored to be a function?

prbly, I look into it, but this file is full of dup code 😵

make `parse` return an object where `base` is a computed property Ref: nodejs#1999

BridgeAR · 2017-09-12T20:25:56Z

Ping @refack

BridgeAR · 2017-09-23T00:18:25Z

Closing due to long inactivity and no response. @refack please feel free to reopen if you would like to pursue this further.

nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Apr 19, 2017

refack added the wip Issues and PRs that are still a work in progress. label Apr 19, 2017

benjamingr added semver-major PRs that contain breaking changes and should be released in the next major version. dont-land-on-v4.x labels Apr 19, 2017

refack force-pushed the improve-path-parse-1999 branch from 4dcae43 to e2cc4e7 Compare April 19, 2017 13:35

refack requested a review from joyeecheung April 19, 2017 15:25

mscdex reviewed Apr 19, 2017

View reviewed changes

refack force-pushed the improve-path-parse-1999 branch from e2cc4e7 to f760d1b Compare April 19, 2017 17:46

refack mentioned this pull request Apr 19, 2017

build: make build faster (at least on Windows) #12281

Closed

8 tasks

joyeecheung reviewed Apr 21, 2017

View reviewed changes

refack force-pushed the improve-path-parse-1999 branch from f760d1b to 3e5c7d0 Compare April 21, 2017 14:55

thefourtheye reviewed Apr 21, 2017

View reviewed changes

refack force-pushed the improve-path-parse-1999 branch 2 times, most recently from 4754c8b to 57272db Compare April 25, 2017 11:53

path: improve parse => format combination

f5d22a2

make `parse` return an object where `base` is a computed property Ref: nodejs#1999

refack force-pushed the improve-path-parse-1999 branch from 57272db to f5d22a2 Compare July 23, 2017 17:45

refack mentioned this pull request Aug 16, 2017

Improve Path.parse / Path.format combo #1999

Closed

BridgeAR added the stalled Issues and PRs that are stalled. label Sep 12, 2017

BridgeAR closed this Sep 23, 2017

refack self-assigned this Nov 11, 2018

refack removed their assignment Mar 11, 2019

refack added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. stalled Issues and PRs that are stalled. and removed stalled Issues and PRs that are stalled. labels May 23, 2019

refack changed the title ~~[wip] path: improve parse => format combination~~ [partial work] path: improve parse => format combination May 23, 2019

refack added the mentor-available label May 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[partial work] path: improve parse => format combination #12511

[partial work] path: improve parse => format combination #12511

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017 •

edited

Loading

benjamingr commented Apr 19, 2017

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017

jasnell commented Apr 19, 2017

benjamingr commented Apr 19, 2017

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017

mscdex Apr 19, 2017 •

edited

Loading

refack Apr 19, 2017

mscdex Apr 19, 2017

mscdex commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017

refack commented Apr 19, 2017

refack commented Apr 19, 2017

nwoltman commented Apr 20, 2017

joyeecheung Apr 21, 2017

refack Apr 21, 2017

thefourtheye Apr 21, 2017

refack Apr 21, 2017

BridgeAR commented Sep 12, 2017

BridgeAR commented Sep 23, 2017

[partial work] path: improve parse => format combination #12511

[partial work] path: improve parse => format combination #12511

Conversation

refack commented Apr 19, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Apr 19, 2017 • edited Loading

benjamingr commented Apr 19, 2017

refack commented Apr 19, 2017 • edited Loading

refack commented Apr 19, 2017

jasnell commented Apr 19, 2017

benjamingr commented Apr 19, 2017

refack commented Apr 19, 2017 • edited Loading

refack commented Apr 19, 2017

mscdex Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

refack Apr 19, 2017

Choose a reason for hiding this comment

mscdex Apr 19, 2017

Choose a reason for hiding this comment

mscdex commented Apr 19, 2017 • edited Loading

refack commented Apr 19, 2017

refack commented Apr 19, 2017

refack commented Apr 19, 2017

nwoltman commented Apr 20, 2017

joyeecheung Apr 21, 2017

Choose a reason for hiding this comment

refack Apr 21, 2017

Choose a reason for hiding this comment

thefourtheye Apr 21, 2017

Choose a reason for hiding this comment

refack Apr 21, 2017

Choose a reason for hiding this comment

BridgeAR commented Sep 12, 2017

BridgeAR commented Sep 23, 2017

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017 •

edited

Loading

refack commented Apr 19, 2017 •

edited

Loading

mscdex Apr 19, 2017 •

edited

Loading

mscdex commented Apr 19, 2017 •

edited

Loading