🚧 Gregor's code review and refactor

.eslintrc.yml

@@ -28,7 +28,6 @@ rules:
   node/no-deprecated-api: 'off' # we still make heavy use of url.parse
 
   # Nock additions.
-  strict: ['error', 'safe']
   no-console: 'error'
   no-loop-func: 'error'
   no-var: 'error'

gregors-code-review-notes

@@ -0,0 +1,2 @@
+- when importing `nock`, `back.setMode("dryrun")` is run by default, which runs `nock.activate()`.
+- `nock.activate()` disables all net requests by default. `back.setMode("dryrun")` enables unmocked net requests explicitly.

index.js

@@ -1,9 +1,9 @@
-'use strict'
+// @ts-check
 
-const back = require('./lib/back')
 const emitter = require('./lib/global_emitter')
 const {
   activate,
+  restore,
   isActive,
   isDone,
   isOn,
@@ -15,12 +15,14 @@ const {
   removeAll,
   abortPendingRequests,
 } = require('./lib/intercept')
+
 const recorder = require('./lib/recorder')
-const { Scope, load, loadDefs, define } = require('./lib/scope')
+const back = require('./lib/back')
 
-module.exports = (basePath, options) => new Scope(basePath, options)
+const { Scope, load, loadDefs, define } = require('./lib/scope')
 
-Object.assign(module.exports, {
+const defaultNockFunction = (basePath, options) => new Scope(basePath, options)
+const API = {
   activate,
   isActive,
   isDone,
@@ -40,9 +42,11 @@ Object.assign(module.exports, {
     clear: recorder.clear,
     play: recorder.outputs,
   },
-  restore: recorder.restore,
+  restore,
   back,
-})
+}
+
+module.exports = Object.assign(defaultNockFunction, API)
 
 // We always activate Nock on import, overriding the globals.
 // Setting the Back mode "activates" Nock by overriding the global entries in the `http/s` modules.

lib/back.js

@@ -4,6 +4,7 @@ const assert = require('assert')
 const recorder = require('./recorder')
 const {
   activate,
+  restore,
   disableNetConnect,
   enableNetConnect,
   removeAll: cleanAll,
@@ -95,6 +96,7 @@ const wild = {
   setup: function () {
     cleanAll()
     recorder.restore()
+    restore()
     activate()
     enableNetConnect()
   },
@@ -112,6 +114,7 @@ const dryrun = {
   setup: function () {
     recorder.restore()
     cleanAll()
+    restore()
     activate()
     //  We have to explicitly enable net connectivity as by default it's off.
     enableNetConnect()
@@ -134,6 +137,7 @@ const record = {
     recorder.restore()
     recorder.clear()
     cleanAll()
+    restore()
     activate()
     disableNetConnect()
   },
@@ -180,6 +184,7 @@ const update = {
     recorder.restore()
     recorder.clear()
     cleanAll()
+    restore()
     activate()
     disableNetConnect()
   },
@@ -221,6 +226,7 @@ const lockdown = {
     recorder.restore()
     recorder.clear()
     cleanAll()
+    restore()
     activate()
     disableNetConnect()
   },

lib/common.js

@@ -1,19 +1,19 @@
-'use strict'
+// @ts-check
+
+const util = require('util')
 
 const debug = require('debug')('nock.common')
 const set = require('lodash.set')
 const timers = require('timers')
-const url = require('url')
-const util = require('util')
 
 /**
  * Normalizes the request options so that it always has `host` property.
  *
  * @param  {Object} options - a parsed options object of the request
  */
 function normalizeRequestOptions(options) {
-  options.proto = options.proto || 'http'
-  options.port = options.port || (options.proto === 'http' ? 80 : 443)
+  options.protocol = options.protocol || 'http:'
+  options.port = options.port || (options.protocol === 'http:' ? 80 : 443)
   if (options.host) {
     debug('options.host:', options.host)
     if (!options.hostname) {
@@ -51,105 +51,26 @@ function isUtf8Representable(buffer) {
   return reconstructedBuffer.equals(buffer)
 }
 
-//  Array where all information about all the overridden requests are held.
-let requestOverrides = {}
-
-/**
- * Overrides the current `request` function of `http` and `https` modules with
- * our own version which intercepts issues HTTP/HTTPS requests and forwards them
- * to the given `newRequest` function.
- *
- * @param  {Function} newRequest - a function handling requests; it accepts four arguments:
- *   - proto - a string with the overridden module's protocol name (either `http` or `https`)
- *   - overriddenRequest - the overridden module's request function already bound to module's object
- *   - options - the options of the issued request
- *   - callback - the callback of the issued request
- */
-function overrideRequests(newRequest) {
-  debug('overriding requests')
-  ;['http', 'https'].forEach(function (proto) {
-    debug('- overriding request for', proto)
-
-    const moduleName = proto // 1 to 1 match of protocol and module is fortunate :)
-    const module = {
-      http: require('http'),
-      https: require('https'),
-    }[moduleName]
-    const overriddenRequest = module.request
-    const overriddenGet = module.get
-
-    if (requestOverrides[moduleName]) {
-      throw new Error(
-        `Module's request already overridden for ${moduleName} protocol.`
-      )
-    }
-
-    //  Store the properties of the overridden request so that it can be restored later on.
-    requestOverrides[moduleName] = {
-      module,
-      request: overriddenRequest,
-      get: overriddenGet,
-    }
-    // https://nodejs.org/api/http.html#http_http_request_url_options_callback
-    module.request = function (input, options, callback) {
-      return newRequest(proto, overriddenRequest.bind(module), [
-        input,
-        options,
-        callback,
-      ])
-    }
-    // https://nodejs.org/api/http.html#http_http_get_options_callback
-    module.get = function (input, options, callback) {
-      const req = newRequest(proto, overriddenGet.bind(module), [
-        input,
-        options,
-        callback,
-      ])
-      req.end()
-      return req
-    }
-
-    debug('- overridden request for', proto)
-  })
-}
-
-/**
- * Restores `request` function of `http` and `https` modules to values they
- * held before they were overridden by us.
- */
-function restoreOverriddenRequests() {
-  debug('restoring requests')
-  Object.entries(requestOverrides).forEach(
-    ([proto, { module, request, get }]) => {
-      debug('- restoring request for', proto)
-      module.request = request
-      module.get = get
-      debug('- restored request for', proto)
-    }
-  )
-  requestOverrides = {}
-}
-
 /**
  * In WHATWG URL vernacular, this returns the origin portion of a URL.
  * However, the port is not included if it's standard and not already present on the host.
  */
-function normalizeOrigin(proto, host, port) {
+function normalizeOrigin(protocol, host, port) {
   const hostHasPort = host.includes(':')
   const portIsStandard =
-    (proto === 'http' && (port === 80 || port === '80')) ||
-    (proto === 'https' && (port === 443 || port === '443'))
+    (protocol === 'http:' && (port === 80 || port === '80')) ||
+    (protocol === 'https:' && (port === 443 || port === '443'))
   const portStr = hostHasPort || portIsStandard ? '' : `:${port}`
 
-  return `${proto}://${host}${portStr}`
+  return `${protocol}//${host}${portStr}`
 }
 
 /**
  * Get high level information about request as string
  * @param  {Object} options
  * @param  {string} options.method
  * @param  {number|string} options.port
- * @param  {string} options.proto Set internally. always http or https
+ * @param  {string} options.protocol Set internally. always http: or https:
  * @param  {string} options.hostname
  * @param  {string} options.path
  * @param  {Object} options.headers
@@ -158,7 +79,7 @@ function normalizeOrigin(proto, host, port) {
  */
 function stringifyRequest(options, body) {
   const { method = 'GET', path = '', port } = options
-  const origin = normalizeOrigin(options.proto, options.hostname, port)
+  const origin = normalizeOrigin(options.protocol, options.hostname, port)
 
   const log = {
     method,
@@ -488,65 +409,6 @@ function isStream(obj) {
   )
 }
 
-/**
- * Converts the arguments from the various signatures of http[s].request into a standard
- * options object and an optional callback function.
- *
- * https://nodejs.org/api/http.html#http_http_request_url_options_callback
- *
- * Taken from the beginning of the native `ClientRequest`.
- * https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/_http_client.js#L68
- */
-function normalizeClientRequestArgs(input, options, cb) {
-  if (typeof input === 'string') {
-    input = urlToOptions(new url.URL(input))
-  } else if (input instanceof url.URL) {
-    input = urlToOptions(input)
-  } else {
-    cb = options
-    options = input
-    input = null
-  }
-
-  if (typeof options === 'function') {
-    cb = options
-    options = input || {}
-  } else {
-    options = Object.assign(input || {}, options)
-  }
-
-  return { options, callback: cb }
-}
-
-/**
- * Utility function that converts a URL object into an ordinary
- * options object as expected by the http.request and https.request APIs.
- *
- * This was copied from Node's source
- * https://github.com/nodejs/node/blob/908292cf1f551c614a733d858528ffb13fb3a524/lib/internal/url.js#L1257
- */
-function urlToOptions(url) {
-  const options = {
-    protocol: url.protocol,
-    hostname:
-      typeof url.hostname === 'string' && url.hostname.startsWith('[')
-        ? url.hostname.slice(1, -1)
-        : url.hostname,
-    hash: url.hash,
-    search: url.search,
-    pathname: url.pathname,
-    path: `${url.pathname}${url.search || ''}`,
-    href: url.href,
-  }
-  if (url.port !== '') {
-    options.port = Number(url.port)
-  }
-  if (url.username || url.password) {
-    options.auth = `${url.username}:${url.password}`
-  }
-  return options
-}
-
 /**
  * Determines if request data matches the expected schema.
  *
@@ -625,11 +487,11 @@ function isPlainObject(value) {
   if (Object.getPrototypeOf(value) === null) {
     return true
   }
-  let proto = value
-  while (Object.getPrototypeOf(proto) !== null) {
-    proto = Object.getPrototypeOf(proto)
+  let prototype = value
+  while (Object.getPrototypeOf(prototype) !== null) {
+    prototype = Object.getPrototypeOf(prototype)
   }
-  return Object.getPrototypeOf(value) === proto
+  return Object.getPrototypeOf(value) === prototype
 }
 
 /**
@@ -698,7 +560,7 @@ function removeAllTimers() {
  * - `request.destroy()`: Added in: v0.3.0
  * - `request.destroyed`: Added in: v14.1.0, v13.14.0
  *
- * @param {ClientRequest} req
+ * @param {import("http").ClientRequest} req
  * @returns {boolean}
  */
 function isRequestDestroyed(req) {
@@ -727,14 +589,11 @@ module.exports = {
   isUtf8Representable,
   mapValue,
   matchStringOrRegexp,
-  normalizeClientRequestArgs,
   normalizeOrigin,
   normalizeRequestOptions,
-  overrideRequests,
   percentDecode,
   percentEncode,
   removeAllTimers,
-  restoreOverriddenRequests,
   setImmediate,
   setInterval,
   setTimeout,