Fails to handle package.json where dependency has version spec of "4.x"

[dtgriscom]

I want to use npmbox to box all dependencies specified in the following package.json:

{
  "name": "XXXXXX",
  "description": "YYYYYYYYY",
  "version": "0.0.1",
  "private": true,
  "dependencies": {
    "express": "4.x",
    "body-parser": "*",
    "multer": "*",
    "hogan-express": "*",
    "ws": "*",
    "snmpjs-new": "*",
    "jsonschema": "*",
    "http-auth": "*",
    "minimist": "*"
  },
  "devDependencies": {
    "@types/node": "6.0.42",
    "ts-node": "1.7.2",
    "tslint": "4.2.0",
    "typescript": "2.0.10"
  }
}

Unfortunately, this fails with npmbox trying to capture a package called "4.x", which is wrong.

The details:

• I run npmbox package.json
• npmbox.js invokes npmboxxer.js's box function
• box starts walking the list of sources by calling box.next
• box.next sees that package.json is a json file, and it should parse the file and push all the mentioned dependencies by calling listDependencies for each type of dependency
• listDependencies takes one of the package.json dependency maps, intending to return a list of full dependency names. It does so by iterating through the dependency map keys, and accumulating the result of calling box.fixDependency with the key and the key value
• box.fixDependency is first called with "express" and "4.x". It invokes npm-package-arg with the latter, and checks whether the returned object has a name property. It does, so box.fixDependency just returns the name property's value, which is "4.x", which is wrong. (If the property hadn't existed, then box.fixDependency would have returned "express" + "@" + "4.x".)
• At the end of the box.next call, the source list ends up with "4.x" as its first item. npmbox then tries to capture the package named "4.x", which is wrong.

The key here seems to be box.fixDependency, which when it is called with "express" and "4.x" should return "[email protected]", but instead returns "4.x", breaking things. I'd be happy to file a pull request. but at this point it would just be to have fixDependency always return its two arguments with "@" between them. However, fixDependency must have been designed to do something; since I have no idea what, any change I'd make would certainly break something else.

Any ideas?

Edit: also happens when I edit the express version spec to "4.1.14", the current version.

[danfuzz]

90% sure I introduced this bug and it represents my misunderstanding of how npm-package-arg works. I won't go so far as to say it's a bug in npm-package-arg but I do think it's at least surprising behavior.

Anyway, I think the right thing to do might just to be to make it always concat name@spec, but IIRC I made that change because of at least some concerns for how that interacted with local path specs (e.g. "foo": "../../pkg/foo" and URL specs (e.g. `"foo": "http://example.com/foo.tgz").

[dtgriscom]

Well, I'm not sure how to proceed. I see that there's some complexity in the handling of the code, but the only thing I could do would be to short-circuit that complexity.

Is npm packageName@versionSpec always the same as a package.json with "packageName": "versionSpec" in the dependencies` array? If not, why and when?

[danfuzz]

but the only thing I could do would be to short-circuit that complexity.

What I meant by my comment was just that. fixDependency() could (probably) be simplified to just return key+"@"+value (and perhaps removed entirely with that concat inlined).

[dtgriscom]

OK, would you like me to make a pull request?

[danfuzz]

I was hoping @arei would have chimed in. I've only ever merged changes that he approved of (either PR approval per se or based on discussion).

The unfortunate facts are that the module doesn't have unit tests, and it's also hard to judge the full extent of the impact of changes to this module in general. On the other hand, if this change were made and it broke someone, npm makes it easy for them to just peg themselves at the earlier version of this module (especially true since this module is by and large a top-level install and not an embedded dependency) until / unless a fix comes in. And as a bit more color, my project (https://github.com/danfuzz/bayou) might actually be the only current user of npmbox with local-path dependencies, and I promise that if this change breaks my project I won't complain to you.

So… I'm comfortable with you submitting a PR, but maybe wait a bit to see what @arei has to say.