path: deprecate internal _makeLong, replace #14956

jasnell · 2017-08-21T05:50:04Z

Replace the internal path._makeLong() with a public path.toNamespacedPath() method. Add documentation.

Refs: standard-things/esm#66

Ping: @ljharb @jdalton

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

mcollina

LGTM

bnoordhuis · 2017-08-21T08:48:31Z

You knew this question was coming: is making it public the right thing to do or should it be spun off into a npm package? _makeLong() is not a privileged function and doesn't do anything special, it just munges strings.

refack

See my comments on jasnell@0222709

refack · 2017-08-21T11:20:07Z

lib/path.js

@@ -1342,11 +1342,10 @@ const posix = {
  },


-  _makeLong: function _makeLong(path) {
+  toLongUNCPath: function toLongUNCPath(path) {


AFAICT to make this a consistent API this needs to be

return posix.resolve(path);

That would change the implementation. That's not what this intends to do.

This is a new API, it need to be consistent.
Since the windows implementation uses resolve either the posix one does too, or remove it from the Windows one.

No, it's not a new API. It literally just renames the existing function and makes the old name a deprecated alias. This api is and has been used by core for a long time and changing its behavior is not something I want to do in this pr.

refack · 2017-08-21T11:27:40Z

You knew this question was coming: is making it public the right thing to do or should it be spun off into a npm package? _makeLong() is not a privileged function and doesn't do anything special, it just munges strings.

IMHO it has value as another endpoint on path, which is anyway just a string munger. Using paths with the \\?\ prefix has "special powers" on Windows e.g. it can include . and .. as directory names, and has a much longer size limitation.
Refs: https://msdn.microsoft.com/library/windows/desktop/aa365247(v=vs.85).aspx

The name is incorrect tough, it should be resolveExtended, since the output in not a UNC path.

jasnell · 2017-08-21T12:13:13Z

@bnoordhuis: simple answer is: node.js needs the function, it's already in the code, it's already accessible, it costs us extremely little to do this. So yes, I think it's the right thing to do.

addaleax · 2017-08-21T12:14:36Z

doc/api/deprecations.md

+<a id="DEP00XX"></a>
+### DEP00XX: path._makeLong()
+
+Type: Runtime


-1 on runtime-deprecating without a reason, especially if we’re exposing an alias

The reason is to strongly discourage userland from making continued use of _-prefixed methods. This is an api they already should not be using, without the deprecation notice, there's no strong motivation for those who are to change.

I don't know if this should be runtime- or docs- deprecated, but I think the deprecation should be in an other commit so we can cherry-pick the addition to v8.x

@jasnell I think I have two issues with that approach:

There should to be a time frame for migration, in which using either name just works, and a run-time deprecation is functional breakage

This PR is committing to supporting this API anyway (which is a good thing in my eyes!). Imho, we should focus runtime-deprecation of APIs, internal or not, on code that can actually be expected to be broken/removed in the future.

A compromise approach, then, would be docs-only in 9.x, runtime deprecation in 10.x.

@targos ... re: cherry-picking, If this goes through, I would open a separate PR against 8.x that can be cherry-picked.

@jasnell I’m okay with that, I guess (not a fan, but eh).

Also, if this is just a docs-deprecation, I’d be in favour of just landing it as a minor in v8.x.

refack · 2017-08-21T12:56:07Z

lib/path.js

@@ -673,7 +674,7 @@ const win32 = {
  },


-  _makeLong: function _makeLong(path) {
+  toLongUNCPath: function toLongUNCPath(path) {


This name is incorrect, the result is not UNC but a specially prefixed "extended lengh" path name.
IMHO best option is resolveExtended (although to keep with dirname and extname is could be all lowercase resolveextended)
Ref: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx

refack · 2017-08-21T12:59:40Z

doc/api/path.md

+* `path` {string}
+* Returns: {string}
+
+On Windows systems only, returns the equivalent UNC path for the given `path`.


The description is imprecise. Also if we decide to go with a resolveExtended interpretation the description needs to change to include a reference to resolve:

Resolves the `path` with [`path.resolve()`][]. On Windows systems only, returns an "extended-length" path, which includes the "\\?\" prefix. For more information see [this MSDN][aa365247] ... [`path.resolve()`]: #path_path_resolve_paths [aa365247]: https://msdn.microsoft.com/library/windows/desktop/aa365247(v=vs.85).aspx

refack · 2017-08-21T14:11:19Z

No, it's not a new API. It literally just renames the existing function and makes the old name a deprecated alias. This api is and has been used by core for a long time and changing its behavior is not something I want to do in this pr.

I get that you want to make it a minimal change, but you can't avoid the fact that documenting it is de facto declaring a new API. New users will become aware of it and will try to figure out how to use it, and for them we need it to make sense and have least amount of surprise.

IMHO simply documenting an internal ad-hoc-ish API has sub-optimal benefit. If we can't find a way to make sense of it, I'd rather it stayed undocumented, or be documented as deprecated.

As I see it the crux of the design issue is that besides prepending \\?\ (which doesn't have a counterpart on POSIX), it also does resolve on Windows (which does have a POSIX counterpart)
https://github.com/nodejs/node/blob/0222709088f03e0d936d4fbf609e81fffd735888/lib/path.js#L686
while on POSIX ~~it's~~ the whole method is a noop, that asymmetry was a surprise to me, so I'm assuming it will surprise others.

jasnell · 2017-08-21T14:16:17Z

while on POSIX it's a noop, that was a surprise to me, so I'm assuming it will surprise others.

which is why it is clearly indicated as a no-op in the added documentation.

refack · 2017-08-21T14:32:16Z

while on POSIX it's a noop, that was a surprise to me, so I'm assuming it will surprise others.

which is why it is clearly indicated as a no-op in the added documentation.

That doesn't answer why it is that way... Or what this API is good for.

Anyway, I am now tending to see this from your POV, but I want to convince myself. Please let me think some more about what you are saying, and about the big picture.

jasnell · 2017-08-21T14:53:38Z

@addaleax ... moved to a docs-only deprecation
@refack ... renamed the public method

ljharb

Would love to see this as semver-minor in 8, and the original name runtime-deprecated in v9 :-)

jasnell · 2017-08-21T15:40:00Z

I will be opening a backport PR to land it as a minor in 8.x, 6.x and 4.x

jasnell · 2017-08-21T15:40:52Z

@refack ... if there are specific implementation changes to the posix version that you'd like to see, please open a separate PR for that.

jasnell · 2017-08-21T16:03:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9776/

bnoordhuis · 2017-08-21T17:45:15Z

simple answer is: node.js needs the function, it's already in the code, it's already accessible, it costs us extremely little to do this. So yes, I think it's the right thing to do.

That's not a great answer. Node has a lot of functions it needs that we don't expose. Why is this function special?

"Costs us extremely little" was the same argument for net.Server#maxConnections, process.on('uncaughtException') and a host of other things we've come to regret later. I'd like to hear a better motivation.

jasnell · 2017-08-21T18:17:04Z

@nodejs/ctc ... please review and weigh in.

ljharb · 2017-08-21T18:33:20Z

It's already exposed. This change just documents it and gives it a better name. If not this, is your suggestion to remove it entirely?

ChALkeR · 2017-09-27T16:59:21Z

@jasnell, doing it right now. Sorry for the long delay on all this…
Btw, I think we need to move the whole gzemnid thing into the org, I got stuck on that, too…

Fishrock123 · 2017-09-27T17:08:54Z

doc/api/deprecations.md

@@ -660,6 +660,15 @@ Type: Runtime

 `REPLServer.parseREPLKeyword()` was removed from userland visibility.

+<a id="DEP00XX"></a>
+### DEP00XX: path._makeLong()


Please escape the underscore.

ChALkeR · 2017-09-27T17:10:57Z

Full grep: https://gist.github.com/ChALkeR/d740faa02146b6d726fedb328c08a043
That is mostly fs copies. Without those, the list is pretty short. I will post a prettier (filtered and weighted) list later today.

+1 to this PR.

Fishrock123 · 2017-09-27T17:11:14Z

I am updating the OP to use the current new API name of path.toNamespacedPath(), rather than path.toLongUNCPath().

Fishrock123

I'm not super keen on exposing more easy-to-implement API but it does also seem reasonable. +-0 with comments addressed.

Fishrock123 · 2017-09-27T17:14:40Z

lib/path.js

@@ -673,7 +673,7 @@ const win32 = {
  },


-  _makeLong: function _makeLong(path) {
+  toNamespacedPath: function toNamespacedPath(path) {


This seems to have rather poor type-checking, can we fix that before exposing it publicly?

Fix in what way? If the value is not a string it simply returns the value. Given how extensively this method is used by other fs methods that perform their own type checking, adding additional type checking to this would be redundant and largely unnecessary.

mhdawson · 2017-09-27T17:16:44Z

Based on discussion in the TSC meeting consensus is that we can move forward. We can land once comments are addressed unless there are objections in next 48 hours.

jasnell · 2017-09-27T17:17:49Z

Ok. Glad to finally see progress on this. I'll get the pr updated

Replace the internal `path._makeLong()` with a public `path.toLongUNCPath()` method. Add documentation. Refs: standard-things/esm#66

jasnell · 2017-10-02T17:47:26Z

PR rebased and updated to address the feedback. Opted not to apply additional type checking to the method. If we want to pursue adding additional type checking, then that can be done as a separate semver-major PR

jasnell · 2017-10-02T17:48:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10380/

TSC decided to move forward with the change. We can add additional type checking later in a separate semver-major PR

refack · 2017-10-02T18:13:28Z

macOS job: https://ci.nodejs.org/job/node-test-commit-osx/12755/nodes=osx1010/

bnoordhuis · 2017-10-02T18:29:38Z

Based on discussion in the TSC meeting consensus is that we can move forward.

@mhdawson The meeting notes in nodejs/TSC#367 don't mention the rationale, only that discussion happened.

Replace the internal `path._makeLong()` with a public `path.toLongUNCPath()` method. Add documentation. PR-URL: #14956 Ref: standard-things/esm#66 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell · 2017-10-02T18:33:02Z

Landed in 1f8d527

jasnell · 2017-10-02T18:33:37Z

@bnoordhuis ... just saw your comment here right after pushing to master. If we need to revisit this, we can, but I was proceeding based on the goahead from TSC and the green CI

bnoordhuis · 2017-10-02T19:42:48Z

@jasnell No, it's fine, I'm just interested to know what the TSC's motivation for moving forward was.

The questions I posted are mentioned in the meeting notes but not if they were actually discussed.

refack · 2017-10-02T21:25:17Z

DEP code left unassigned.
I'm thinking of a way to lint for this 🤔

jasnell · 2017-10-02T21:31:58Z

Pr to fix the dep code is already open.
There is a check made already during the release process. A tool such as @evanlucas' commit checker could be modified to check for this.

refack · 2017-10-02T21:36:50Z

A tool such as @evanlucas' commit checker could be modified to check for this.

I was think about something that wraps core-validate-commit:

Find the the changeset since upstream/master
Loop and run core-validate-commit on all commits message
Run lint on all changed files
Special lint rules

I'm open a PR and if anyone has suggestions they should add there.

Replace the internal `path._makeLong()` with a public `path.toLongUNCPath()` method. Add documentation. PR-URL: nodejs/node#14956 Ref: standard-things/esm#66 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

jasnell added notable-change PRs with changes that should be highlighted in changelogs. path Issues and PRs related to the path subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 21, 2017

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. path Issues and PRs related to the path subsystem. labels Aug 21, 2017

jasnell requested review from MylesBorins, addaleax and a team August 21, 2017 05:50

mcollina approved these changes Aug 21, 2017

View reviewed changes

refack suggested changes Aug 21, 2017

View reviewed changes

addaleax requested changes Aug 21, 2017

View reviewed changes

refack suggested changes Aug 21, 2017

View reviewed changes

addaleax approved these changes Aug 21, 2017

View reviewed changes

ljharb approved these changes Aug 21, 2017

View reviewed changes

refack approved these changes Aug 21, 2017

View reviewed changes

jasnell added the ctc-review label Aug 21, 2017

mhdawson mentioned this pull request Sep 25, 2017

Node.js Foundation Technical Steering Committee (TSC) Meeting 2017-09-27 nodejs/TSC#363

Closed

Trott removed the tsc-review label Sep 26, 2017

Fishrock123 reviewed Sep 27, 2017

View reviewed changes

ChALkeR self-requested a review September 27, 2017 17:12

Fishrock123 previously requested changes Sep 27, 2017

View reviewed changes

BridgeAR removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 27, 2017

path: deprecate internal _makeLong, replace

9277e4a

Replace the internal `path._makeLong()` with a public `path.toLongUNCPath()` method. Add documentation. Refs: standard-things/esm#66

jasnell force-pushed the deprecate-internal-makelong branch from 656f162 to 9277e4a Compare October 2, 2017 17:46

refack force-pushed the master branch from 22f95e7 to 7069e63 Compare October 2, 2017 17:48

jasnell closed this Oct 2, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

path: deprecate internal _makeLong, replace #14956

path: deprecate internal _makeLong, replace #14956

Conversation

jasnell commented Aug 21, 2017 • edited by Fishrock123 Loading

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

bnoordhuis commented Aug 21, 2017

refack 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

refack commented Aug 21, 2017

jasnell commented Aug 21, 2017

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

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

refack commented Aug 21, 2017 • edited Loading

jasnell commented Aug 21, 2017

refack commented Aug 21, 2017

jasnell commented Aug 21, 2017

ljharb left a comment

Choose a reason for hiding this comment

jasnell commented Aug 21, 2017

jasnell commented Aug 21, 2017

jasnell commented Aug 21, 2017

bnoordhuis commented Aug 21, 2017

jasnell commented Aug 21, 2017

ljharb commented Aug 21, 2017

ChALkeR commented Sep 27, 2017

Choose a reason for hiding this comment

ChALkeR commented Sep 27, 2017 • edited Loading

Fishrock123 commented Sep 27, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Sep 27, 2017

jasnell commented Sep 27, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

refack commented Oct 2, 2017

bnoordhuis commented Oct 2, 2017

jasnell commented Oct 2, 2017

jasnell commented Oct 2, 2017

bnoordhuis commented Oct 2, 2017

refack commented Oct 2, 2017

jasnell commented Oct 2, 2017

refack commented Oct 2, 2017

jasnell commented Aug 21, 2017 •

edited by Fishrock123

Loading

refack commented Aug 21, 2017 •

edited

Loading

ChALkeR commented Sep 27, 2017 •

edited

Loading