Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix for pkg.name fields that aren't requirable #2538

Closed
wants to merge 3 commits into from

2 participants

@substack

Sometimes a package.json will have a "name" field set but won't be in a node_modules directory or will have a different basename than the name field, rendering the package unrequirable by subdependencies that wrongly assume the package is already available and won't pull down a local version from the registry.

I encountered this problem running the tests for js-traverse, which uses tap, but tap depends on difflet which depends on traverse. npm didn't fetch traverse as a dependency for difflet, causing my tests to fail with: Couldn't find module "traverse"

This patch tacks on a tap test into test/tap and adds && tap tests/tap/*.js to the scripts.test since the existing test stuff seemed non-obvious where to start with. The change itself is a relatively minor yet hackish check in lib/install.js that tacks on some random garbage to the end of the data.name so that subdependencies won't wrongly assume they already have a package that they won't be able to require(). There's probably a better way to fix this but this way works and all the existing tests pass.

@substack

Updated with a less hackish version that I can't even test because the registry is crazy slow right now.

Update: didn't actually work so I force pushed over that commit.

@substack

Setting to null seems like a less-hackish alternative that actually works.

@isaacs isaacs closed this pull request from a commit
@substack substack Fix #2538 Properly ignore false ancestors
When running `npm install` in a package, if one of the deps depends on
the root package, it might not be require()-able (if the folder name
doens't match, and/or if it's not in a node_modules folder).

This makes that work.

Originally by @SubStack, edited slightly by @isaacs.

* failing test for pkg.name that can't be required
* fix the false_name test by altering data.name when it's not in a
  requirable location
* set data.name to null for unrequirable packages, less hackishly
9f9d4a5
@isaacs isaacs closed this in 9f9d4a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
8 lib/install.js
@@ -127,6 +127,14 @@ function install (args, cb_) {
, explicit: false
, parent: data
, wrap: null }
+
+ if (data.name !== path.basename(where) ||
+ path.basename(path.dirname(where)) !== 'node_modules') {
+ // there's a pkg.name, but it can't be required, so shim the name out
+ // to something unresolvable so the real deps will resolve
+ data.name = null
+ }
+
context.family[data.name] = context.ancestors[data.name] = data.version
installManyTop(deps.map(function (dep) {
var target = data.dependencies[dep]
View
5 package.json
@@ -102,14 +102,15 @@
"osenv"
],
"devDependencies": {
- "ronn": "https://github.com/isaacs/ronnjs/tarball/master"
+ "ronn": "https://github.com/isaacs/ronnjs/tarball/master",
+ "tap": "~0.2.5"
},
"engines": {
"node": "0.6 || 0.7 || 0.8",
"npm": "1"
},
"scripts": {
- "test": "node ./test/run.js",
+ "test": "node ./test/run.js && tap test/tap/*.js",
"prepublish": "npm prune ; make -j4 doc",
"dumpconf": "env | grep npm | sort | uniq"
},
View
29 test/tap/false_name.js
@@ -0,0 +1,29 @@
+var test = require('tap').test
+ , fs = require('fs')
+ , path = require('path')
+ , existsSync = fs.existsSync || path.existsSync
+ , spawn = require('child_process').spawn
+ , npm = require('../../')
+
+test('not every pkg.name can be required', function (t) {
+ t.plan(1)
+
+ setup(function () {
+ npm.install('.', function (err) {
+ if (err) return t.fail(err)
+ t.ok(existsSync(__dirname +
+ '/false_name/node_modules/tap/node_modules/buffer-equal'))
+ })
+ })
+})
+
+function setup (cb) {
+ process.chdir(__dirname + '/false_name')
+ npm.load(function () {
+ spawn('rm', [ '-rf', __dirname + '/false_name/node_modules' ])
+ .on('exit', function () {
+ fs.mkdirSync(__dirname + '/false_name/node_modules')
+ cb()
+ })
+ })
+}
View
1  test/tap/false_name/index.js
@@ -0,0 +1 @@
+module.exports = true
View
8 test/tap/false_name/package.json
@@ -0,0 +1,8 @@
+{
+ "name": "buffer-equal",
+ "version": "0.0.0",
+ "main": "index.js",
+ "dependencies": {
+ "tap": "0.2.5"
+ }
+}
Something went wrong with that request. Please try again.