fixup! windows paths and a bunch of encoding nonsense

Author: wraithgar

README.md

@@ -8,22 +8,22 @@ Parses package name and specifier passed to commands like `npm install` or
 ## EXAMPLES
 
 ```javascript
-var assert = require("assert")
-var npa = require("npm-package-arg")
+const assert = require("assert")
+const npa = require("npm-package-arg")
 
 // Pass in the descriptor, and it'll return an object
 try {
-  var parsed = npa("@bar/[email protected]")
+  const parsed = npa("@bar/[email protected]")
 } catch (ex) {
   …
 }
 ```
 
 ## USING
 
-`var npa = require('npm-package-arg')`
+`const npa = require('npm-package-arg')`
 
-### var result = npa(*arg*[, *where*])
+### const result = npa(*arg*[, *where*])
 
 * *arg* - a string that you might pass to `npm install`, like:
 `[email protected]`, `@bar/[email protected]`, `foo@user/foo`, `http://x.com/foo.tgz`,
@@ -34,7 +34,7 @@ part, eg `foo` then the specifier will default to `latest`.
 
 **Throws** if the package name is invalid, a dist-tag is invalid or a URL's protocol is not supported.
 
-### var result = npa.resolve(*name*, *spec*[, *where*])
+### const result = npa.resolve(*name*, *spec*[, *where*])
 
 * *name* - The name of the module you want to install. For example: `foo` or `@bar/foo`.
 * *spec* - The specifier indicating where and how you can get this module. Something like:
@@ -45,7 +45,7 @@ included then the default is `latest`.
 
 **Throws** if the package name is invalid, a dist-tag is invalid or a URL's protocol is not supported.
 
-### var purl = npa.toPurl(*arg*, *reg*)
+### const purl = npa.toPurl(*arg*, *reg*)
 
 Returns the [purl (package URL)](https://github.com/package-url/purl-spec) form of the given package name/spec.
 
@@ -79,9 +79,9 @@ keys:
   specification. Mostly used when making requests against a registry. When
   `name` is `null`, `escapedName` will also be `null`.
 * `rawSpec` - The specifier part that was parsed out in calls to `npa(arg)`,
-  or the value of `spec` in calls to `npa.resolve(name, spec).
+  or the value of `spec` in calls to `npa.resolve(name, spec)`.
 * `saveSpec` - The normalized specifier, for saving to package.json files.
-  `null` for registry dependencies.
+  `null` for registry dependencies. See note below about how this is (not) encoded.
 * `fetchSpec` - The version of the specifier to be used to fetch this
   resource.  `null` for shortcuts to hosted git dependencies as there isn't
   just one URL to try with them.
@@ -94,3 +94,11 @@ keys:
   `npa.resolve(name, spec)` then this will be `name + '@' + spec`.
 * `subSpec` - If `type === 'alias'`, this is a Result Object for parsing the
   target specifier for the alias.
+
+## SAVE SPECS
+
+TLDR: `file:` urls are NOT uri encoded.
+
+Historically, npm would uri decode file package args, but did not do any uri encoding for the `saveSpec`.  This meant that it generated incorrect saveSpecs for directories with characters that *looked* like encoded uri characters, and also that it could not parse directories with some unencoded uri characters (such as `%`).
+
+In order to fix this, and to not break all existing versions of npm, this module now parses all file package args as not being uri encoded. And in order to not break all of the package.json files npm has made in the past,  it also does not uri encode the saveSpec.  This includes package args that start with `file:`.  This does mean that npm `file:` package args are not RFC compliant, and making them so constitutes quite a breaking change.

lib/npa.js

@@ -3,7 +3,8 @@
 const isWindows = process.platform === 'win32'
 
 const { URL } = require('node:url')
-const path = isWindows ? require('node:path').win32 : require('node:path')
+// We need to use path/win32 so that we get consistent results in tests, but this also means we need to manually convert backslashes to forward slashes when generating file: urls with paths.
+const path = isWindows ? require('node:path/win32') : require('node:path')
 const { homedir } = require('node:os')
 const HostedGit = require('hosted-git-info')
 const semver = require('semver')
@@ -276,39 +277,17 @@ function pathToFileURL (str) {
   for (let i = 0; i < str.length; i++) {
     result = `${result}${encodedPathChars.get(str[i]) ?? str[i]}`
   }
+  if (result.startsWith('file:')) {
+    return result
+  }
   return `file:${result}`
 }
 
-/* parse file package args:
- *
- * /posix/path
- * ./posix/path
- * .dotfile
- * .dot/path
- * filename
- * filename.with.ext
- * C:\windows\path
- * path/with/no/leading/separator
- *
- * translates to relative ./path
- *  - file:{path}
- *
- * translates to absolute /path
- * - file:/{path}
- * - file://{path} (this is not RFC compliant, but is supported for backward compatibility)
- * - file:///{path}
- *
- * file: specs are url encoded, bare paths are not
- *
- */
 function fromFile (res, where) {
   res.type = isFileType.test(res.rawSpec) ? 'file' : 'directory'
   res.where = where
 
-  let rawSpec = res.rawSpec
-  if (!rawSpec.startsWith('file:')) {
-    rawSpec = pathToFileURL(rawSpec)
-  }
+  let rawSpec = pathToFileURL(res.rawSpec)
 
   if (rawSpec.startsWith('file:/')) {
     // XXX backwards compatibility lack of compliance with RFC 8089
@@ -361,6 +340,8 @@ function fromFile (res, where) {
   }
 
   res.fetchSpec = path.resolve(where, resolvedPath)
+  // re-normalize the slashes in saveSpec due to node:path/win32 behavior in windows
+  res.saveSpec = res.saveSpec.split('\\').join('/')
   return res
 }
 

test/basic.js

@@ -6,7 +6,6 @@ const normalizePath = p => p && p.replace(/^[a-zA-Z]:/, '').replace(/\\/g, '/')
 const cwd = normalizePath(process.cwd())
 process.cwd = () => cwd
 const normalizePaths = spec => {
-  spec.saveSpec = normalizePath(spec.saveSpec)
   spec.fetchSpec = normalizePath(spec.fetchSpec)
   return spec
 }
@@ -15,8 +14,6 @@ const t = require('tap')
 const npa = t.mock('..', { path })
 
 t.test('basic', function (t) {
-  t.setMaxListeners(999)
-
   const tests = {
     '[email protected]': {
       name: 'foo',