Refactor PR 533 and Fix issue 653

- Moved logic regarding host selection to versioning where all user defined values from package.json are transformed into command options.
- Moved testing of feature from `run.test.js` to `versioning.test.js`.
- Added `development_host` option. Becomes default option for `publish` `unpublish` when present.
- Changed behavior when alternate hosts are defined. Now `production_host` acts as alias to host. Defining `staging_host` or `development_host` is enough to default `publish` and `unpublish` away from production.
- When a chain of commands that includes `publish` or `unpublish`, when host not specifically set via command line or environment variable, ALL commands in the chain default away from production.
- An invalid `s3_host` option does not result in error and is instead silently ignored.
- Change is backwards compatible with previously valid configurations.

Author: ronilan

lib/main.js

@@ -72,12 +72,6 @@ function run() {
     return;
   }
 
-  // set binary.host when appropriate. host determines the s3 target bucket.
-  const target = prog.setBinaryHostProperty(command.name);
-  if (target && ['install', 'publish', 'unpublish', 'info'].indexOf(command.name) >= 0) {
-    log.info('using binary.host: ' + prog.package_json.binary.host);
-  }
-
   prog.commands[command.name](command.args, function(err) {
     if (err) {
       log.error(command.name + ' error');

lib/node-pre-gyp.js

@@ -73,11 +73,8 @@ function Run({ package_json_path = './package.json', argv }) {
   });
 
   this.parseArgv(argv);
-
-  // this is set to true after the binary.host property was set to
-  // either staging_host or production_host.
-  this.binaryHostSet = false;
 }
+
 inherits(Run, EE);
 exports.Run = Run;
 const proto = Run.prototype;
@@ -201,67 +198,6 @@ proto.parseArgv = function parseOpts(argv) {
   log.resume();
 };
 
-/**
- * allow the binary.host property to be set at execution time.
- *
- * for this to take effect requires all the following to be true.
- * - binary is a property in package.json
- * - binary.host is falsey
- * - binary.staging_host is not empty
- * - binary.production_host is not empty
- *
- * if any of the previous checks fail then the function returns an empty string
- * and makes no changes to package.json's binary property.
- *
- *
- * if command is "publish" then the default is set to "binary.staging_host"
- * if command is not "publish" the the default is set to "binary.production_host"
- *
- * if the command-line option '--s3_host' is set to "staging" or "production" then
- * "binary.host" is set to the specified "staging_host" or "production_host". if
- * '--s3_host' is any other value an exception is thrown.
- *
- * if '--s3_host' is not present then "binary.host" is set to the default as above.
- *
- * this strategy was chosen so that any command other than "publish" or "unpublish" uses "production"
- * as the default without requiring any command-line options but that "publish" and "unpublish" require
- * '--s3_host production_host' to be specified in order to *really* publish (or unpublish). publishing
- * to staging can be done freely without worrying about disturbing any production releases.
- */
-proto.setBinaryHostProperty = function(command) {
-  if (this.binaryHostSet) {
-    return this.package_json.binary.host;
-  }
-  const p = this.package_json;
-  // don't set anything if host is present. it must be left blank to trigger this.
-  if (!p || !p.binary || p.binary.host) {
-    return '';
-  }
-  // and both staging and production must be present. errors will be reported later.
-  if (!p.binary.staging_host || !p.binary.production_host) {
-    return '';
-  }
-  let target = 'production_host';
-  if (command === 'publish' || command === 'unpublish') {
-    target = 'staging_host';
-  }
-  // the environment variable has priority over the default or the command line. if
-  // either the env var or the command line option are invalid throw an error.
-  const npg_s3_host = process.env.node_pre_gyp_s3_host;
-  if (npg_s3_host === 'staging' || npg_s3_host === 'production') {
-    target = `${npg_s3_host}_host`;
-  } else if (this.opts['s3_host'] === 'staging' || this.opts['s3_host'] === 'production') {
-    target = `${this.opts['s3_host']}_host`;
-  } else if (this.opts['s3_host'] || npg_s3_host) {
-    throw new Error(`invalid s3_host ${this.opts['s3_host'] || npg_s3_host}`);
-  }
-
-  p.binary.host = p.binary[target];
-  this.binaryHostSet = true;
-
-  return p.binary.host;
-};
-
 /**
  * Returns the usage instructions for node-pre-gyp.
  */

lib/pre-binding.js

@@ -19,7 +19,6 @@ exports.find = function(package_json_path, opts) {
     throw new Error(package_json_path + 'does not exist');
   }
   const prog = new npg.Run({ package_json_path, argv: process.argv });
-  prog.setBinaryHostProperty();
   const package_json = prog.package_json;
 
   versioning.validate_config(package_json, opts);

lib/util/versioning.js

@@ -186,12 +186,6 @@ function get_runtime_abi(runtime, target_version) {
 }
 module.exports.get_runtime_abi = get_runtime_abi;
 
-const required_parameters = [
-  'module_name',
-  'module_path',
-  'host'
-];
-
 function validate_config(package_json, opts) {
   const msg = package_json.name + ' package.json is not node-pre-gyp ready:\n';
   const missing = [];
@@ -207,25 +201,33 @@ function validate_config(package_json, opts) {
   if (!package_json.binary) {
     missing.push('binary');
   }
-  const o = package_json.binary;
-  if (o) {
-    required_parameters.forEach((p) => {
-      if (!o[p] || typeof o[p] !== 'string') {
-        missing.push('binary.' + p);
-      }
-    });
+
+  if (package_json.binary) {
+    if (!package_json.binary.module_name) {
+      missing.push('binary.module_name');
+    }
+    if (!package_json.binary.module_path) {
+      missing.push('binary.module_path');
+    }
+    if (!package_json.binary.host && !package_json.binary.production_host) {
+      missing.push('binary.host');
+    }
   }
 
   if (missing.length >= 1) {
     throw new Error(msg + 'package.json must declare these properties: \n' + missing.join('\n'));
   }
-  if (o) {
-    // enforce https over http
-    const protocol = url.parse(o.host).protocol;
-    if (protocol === 'http:') {
-      throw new Error("'host' protocol (" + protocol + ") is invalid - only 'https:' is accepted");
-    }
+
+  if (package_json.binary) {
+    // for all possible host definitions - verify https usage
+    ['host', 'production_host', 'staging_host', 'development_host'].filter((item) => package_json.binary[item]).forEach((item) => {
+      const protocol = url.parse(package_json.binary[item]).protocol;
+      if (protocol === 'http:') {
+        throw new Error(msg + "'" + item + "' protocol (" + protocol + ") is invalid - only 'https:' is accepted");
+      }
+    });
   }
+
   napi.validate_package_json(package_json, opts);
 }
 
@@ -309,11 +311,46 @@ module.exports.evaluate = function(package_json, options, napi_build_version) {
     region: package_json.binary.region,
     s3ForcePathStyle: package_json.binary.s3ForcePathStyle || false
   };
-    // support host mirror with npm config `--{module_name}_binary_host_mirror`
-    // e.g.: https://github.com/node-inspector/v8-profiler/blob/master/package.json#L25
-    // > npm install v8-profiler --profiler_binary_host_mirror=https://registry.npmmirror.com/node-inspector/
+
+  // user can define a target host key to use (development_host, staging_host, production_host)
+  // by  setting the name of the host (development, staging, production)
+  // into an environment variable or via a command line option.
+  // the environment variable has priority over the the command line.
+  let targetHost = process.env.node_pre_gyp_s3_host || options.s3_host;
+
+  // if value is not one of the allowed or the matching key is not found in package.json
+  // silently ignore the option
+  if (['production', 'staging', 'development'].indexOf(targetHost) === -1 || !package_json.binary[`${targetHost}_host`]) {
+    targetHost = '';
+  }
+
+  // the production host is as specified in 'host' key (default)
+  // unless there is none and alias production_host is specified (backwards compatibility)
+  // note: package.json is verified in validate_config to include at least one of the two.
+  let host = package_json.binary.host ||  package_json.binary.production_host;
+
+  // when a valid target is specified by user, the host is from that target (or 'host')
+  if (targetHost === 'staging') {
+    host = package_json.binary.staging_host;
+  } else if (targetHost === 'development') {
+    host = package_json.binary.development_host;
+  } else if (!targetHost && (package_json.binary.development_host || package_json.binary.staging_host)) {
+    // when host not specifically set via command line or environment variable
+    // but staging and/or development host are present in package.json
+    // for any command (or command chain) that includes publish or unpublish
+    // default to lower host (development, and if not preset, staging).
+    if (options.argv && options.argv.remain.some((item) => (item === 'publish' || item === 'unpublish'))) {
+      host = package_json.binary.development_host || package_json.binary.staging_host;
+    }
+  }
+
+  // support host mirror with npm config `--{module_name}_binary_host_mirror`
+  // e.g.: https://github.com/node-inspector/v8-profiler/blob/master/package.json#L25
+  // > npm install v8-profiler --profiler_binary_host_mirror=https://npm.taobao.org/mirrors/node-inspector/
   const validModuleName = opts.module_name.replace('-', '_');
-  const host = process.env['npm_config_' + validModuleName + '_binary_host_mirror'] || package_json.binary.host;
+  // explicitly set mirror overrides everything set above
+  host = process.env['npm_config_' + validModuleName + '_binary_host_mirror'] || host;
+
   opts.host = fix_slashes(eval_template(host, opts));
   opts.module_path = eval_template(package_json.binary.module_path, opts);
   // now we resolve the module_path to ensure it is absolute so that binding.gyp variables work predictably

test/run.test.js

@@ -28,10 +28,6 @@ const package_json_template = {
   }
 };
 
-
-const all_commands = ['build', 'clean', 'configure', 'info', 'install', 'package', 'publish', 'rebuild',
-  'reinstall', 'reveal', 'testbinary', 'testpackage', 'unpublish'];
-
 /**
  * before testing create a scratch directory to run tests in.
  */
@@ -67,82 +63,6 @@ test.onFinish(() => {
   rimraf(scratch).then(() => undefined, () => undefined);
 });
 
-test('should set staging and production hosts', (t) => {
-  // make sure it's good when specifying host.
-  const mock_package_json = makePackageJson();
-
-  let { prog } = setupTest(dir, mock_package_json);
-  t.deepEqual(prog.package_json, mock_package_json);
-  t.equal(prog.binaryHostSet, false, 'binary host should not be flagged as set');
-
-  // test with no s3_host option
-  all_commands.forEach((cmd) => {
-    const mpj = clone(mock_package_json);
-    mpj.binary.host = '';
-    const opts = { argv: [cmd] };
-    ({ prog } = setupTest(dir, mpj, opts));
-    mpj.binary.host = (cmd === 'publish' || cmd === 'unpublish') ? mpj.binary.staging_host : mpj.binary.production_host;
-    t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
-    t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
-  });
-
-  // test with s3_host set to staging
-  all_commands.forEach((cmd) => {
-    const mpj = clone(mock_package_json);
-    mpj.binary.host = '';
-    const opts = { argv: [cmd, '--s3_host=staging'] };
-    ({ prog } = setupTest(dir, mpj, opts));
-    mpj.binary.host = mpj.binary.staging_host;
-    t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
-    t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
-  });
-
-  // test with s3_host set to production
-  all_commands.forEach((cmd) => {
-    const mpj = clone(mock_package_json);
-    mpj.binary.host = '';
-    const opts = { argv: [cmd, '--s3_host=production'] };
-    ({ prog } = setupTest(dir, mpj, opts));
-    mpj.binary.host = mpj.binary.production_host;
-    t.deepEqual(prog.package_json, mpj, 'host should be correct for command: ' + cmd);
-    t.equal(prog.binaryHostSet, true, 'binary host should be flagged as set');
-  });
-
-  t.end();
-});
-
-test('should execute setBinaryHostProperty() properly', (t) => {
-  // it only --s3_host only takes effect if host is falsey.
-  const mock_package_json = makePackageJson({ binary: { host: '' } });
-
-  const opts = { argv: ['publish', '--s3_host=staging'] };
-
-  let { prog, binaryHost } = setupTest(dir, mock_package_json, opts);
-  t.equal(binaryHost, mock_package_json.binary.staging_host);
-
-  // set it again to verify that it returns the already set value
-  binaryHost = prog.setBinaryHostProperty('publish');
-  t.equal(binaryHost, mock_package_json.binary.staging_host);
-
-  // now do this again but expect an empty binary host value because
-  // staging_host is missing.
-  const mpj = clone(mock_package_json);
-  delete mpj.binary.staging_host;
-  ({ prog, binaryHost } = setupTest(dir, mpj, opts));
-  t.equal(binaryHost, '');
-
-  // one more time but with an invalid value for s3_host
-  opts.argv = ['publish', '--s3_host=bad-news'];
-  try {
-    ({ prog, binaryHost } = setupTest(dir, mock_package_json, opts));
-    t.fail('should throw with --s3_host=bad-news');
-  } catch (e) {
-    t.equal(e.message, 'invalid s3_host bad-news');
-  }
-
-  t.end();
-});
-
 test('verify that the --directory option works', (t) => {
   const initial = process.cwd();
 
@@ -223,6 +143,10 @@ test('verify that a non-existent package.json fails', (t) => {
 // test helpers.
 //
 
+// helper to clone mock package.json.
+// // https://stackoverflow.com/questions/4459928/how-to-deep-clone-in-javascript
+const clone = (obj) => JSON.parse(JSON.stringify(obj));
+
 function makePackageJson(options = {}) {
   const package_json = clone(package_json_template);
   // override binary values if supplied
@@ -233,60 +157,3 @@ function makePackageJson(options = {}) {
   }
   return package_json;
 }
-
-// helper to write package.json to disk so Run() can be instantiated with it.
-function setupTest(directory, package_json, opts) {
-  opts = opts || {};
-  let argv = ['node', 'program'];
-  if (opts.argv) {
-    argv = argv.concat(opts.argv);
-  }
-  const prev_dir = process.cwd();
-  if (!opts.noChdir) {
-    try {
-      fs.mkdirSync(directory);
-    } catch (e) {
-      if (e.code !== 'EEXIST') {
-        throw e;
-      }
-    }
-    process.chdir(directory);
-  }
-
-  try {
-    fs.writeFileSync('package.json', JSON.stringify(package_json));
-    const prog = new npg.Run({ package_json_path: './package.json', argv });
-    const binaryHost = prog.setBinaryHostProperty(prog.todo[0] && prog.todo[0].name);
-    return { prog, binaryHost };
-  } finally {
-    process.chdir(prev_dir);
-  }
-}
-
-// helper to clone mock package.json. it's overkill for existing tests
-// but is future-proof.
-// https://stackoverflow.com/questions/4459928/how-to-deep-clone-in-javascript
-function clone(obj, hash = new WeakMap()) {
-  if (Object(obj) !== obj) return obj;      // primitives
-  if (hash.has(obj)) return hash.get(obj);  // cyclic reference
-  let result;
-
-  if (obj instanceof Set) {
-    result = new Set(obj);                  // treat set as a value
-  } else if (obj instanceof Map) {
-    result = new Map(Array.from(obj, ([key, val]) => [key, clone(val, hash)]));
-  } else if (obj instanceof Date) {
-    result = new Date(obj);
-  } else if (obj instanceof RegExp) {
-    result = new RegExp(obj.source, obj.flags);
-  } else if (obj.constructor) {
-    result = new obj.constructor();
-  } else {
-    result = Object.create(null);
-  }
-  hash.set(obj, result);
-  return Object.assign(result, ...Object.keys(obj).map((key) => {
-    return { [key]: clone(obj[key], hash) };
-  }));
-}
-