refactor: remove unused, duplicated code in deprecate module (#14579)

* remove obsolete tests

 * remove unused deprecate API

 * make a warnOnce helper for the deprecate methods

 * misc. copyediting, e.g. variable names, whitespace

 * test that any deprecation warns once at most

 * use strict

lib/browser/api/app.js

@@ -44,7 +44,9 @@ const nativeFn = app.getAppMetrics
 app.getAppMetrics = () => {
   let metrics = nativeFn.call(app)
   for (const metric of metrics) {
-    deprecate.removeProperty(metric, 'memory', true)
+    if ('memory' in metric) {
+      deprecate.removeProperty(metric, 'memory')
+    }
   }
 
   return metrics

lib/common/api/deprecate.js

@@ -1,6 +1,20 @@
+'use strict'
 
 let deprecationHandler = null
 
+function warnOnce (oldName, newName) {
+  let warned = false
+  const msg = newName
+    ? `'${oldName}' is deprecated and will be removed. Please use '${newName}' instead.`
+    : `'${oldName}' is deprecated and will be removed.`
+  return () => {
+    if (!warned && !process.noDeprecation) {
+      warned = true
+      deprecate.log(msg)
+    }
+  }
+}
+
 const deprecate = {
   setHandler: (handler) => { deprecationHandler = handler },
   getHandler: () => deprecationHandler,
@@ -18,100 +32,61 @@ const deprecate = {
       return console.warn(`(electron) ${message}`)
     }
   },
-  renameFunction: function (fn, oldName, newName) {
-    let warned = false
-    return function () {
-      if (!(warned || process.noDeprecation)) {
-        warned = true
-        deprecate.warn(oldName, newName)
-      }
-      return fn.apply(this, arguments)
-    }
-  },
-  removeFunction: (oldName) => {
-    if (!process.noDeprecation) {
-      deprecate.log(`The '${oldName}' function has been deprecated and marked for removal.`)
-    }
-  },
-  alias: function (object, oldName, newName) {
-    let warned = false
-    const newFn = function () {
-      if (!(warned || process.noDeprecation)) {
-        warned = true
-        deprecate.warn(oldName, newName)
-      }
-      return this[newName].apply(this, arguments)
-    }
-    if (typeof object === 'function') {
-      object.prototype[newName] = newFn
-    } else {
-      object[oldName] = newFn
-    }
-  },
-  event: (emitter, oldName, newName) => {
-    let warned = false
-    return emitter.on(newName, function (...args) {
-      if (this.listenerCount(oldName) === 0) return
-      if (warned || process.noDeprecation) return
 
-      warned = true
+  event: (emitter, oldName, newName) => {
-      if (newName.startsWith('-')) {
+    const warn = newName.startsWith('-') /* internal event */
-        deprecate.log(`'${oldName}' event has been deprecated.`)
+      ? warnOnce(`${oldName} event`)
-      } else {
+      : warnOnce(`${oldName} event`, `${newName} event`)
-        deprecate.warn(`'${oldName}' event`, `'${newName}' event`)
+    return emitter.on(newName, function (...args) {
-      }
+      if (this.listenerCount(oldName) !== 0) {
+        warn()
         this.emit(oldName, ...args)
+      }
     })
   },
-  removeProperty: (object, deprecated, ignoreMissingProps = false) => {
+
-    let warned = false
+  removeProperty: (o, removedName) => {
-    let warn = () => {
+    // if the property's already been removed, warn about it
-      if (!(warned || process.noDeprecation)) {
+    if (!(removedName in o)) {
-        warned = true
+      deprecate.log(`Unable to remove property '${removedName}' from an object that lacks it.`)
-        deprecate.log(`The '${deprecated}' property has been deprecated and marked for removal.`)
-      }
     }
 
-    if (!(deprecated in object)) {
+    // wrap the deprecated property in an accessor to warn
-      if (ignoreMissingProps) return
+    const warn = warnOnce(removedName)
-      throw new Error('Cannot deprecate a property on an object which does not have that property')
+    let val = o[removedName]
-    }
+    return Object.defineProperty(o, removedName, {
-
-    let temp = object[deprecated]
-    return Object.defineProperty(object, deprecated, {
       configurable: true,
       get: () => {
         warn()
-        return temp
+        return val
       },
-      set: (newValue) => {
+      set: newVal => {
         warn()
-        temp = newValue
+        val = newVal
       }
     })
   },
-  renameProperty: (object, oldName, newName) => {
+
-    let warned = false
+  renameProperty: (o, oldName, newName) => {
-    let warn = () => {
+    const warn = warnOnce(oldName, newName)
-      if (!(warned || process.noDeprecation)) {
+
-        warned = true
+    // if the new property isn't there yet,
-        deprecate.warn(oldName, newName)
+    // inject it and warn about it
-      }
+    if ((oldName in o) && !(newName in o)) {
+      warn()
+      o[newName] = o[oldName]
     }
 
-    if (!(newName in object) && (oldName in object)) {
+    // wrap the deprecated property in an accessor to warn
+    // and redirect to the new property
+    return Object.defineProperty(o, oldName, {
+      get: () => {
         warn()
-      object[newName] = object[oldName]
+        return o[newName]
-    }
-
-    return Object.defineProperty(object, oldName, {
-      get: function () {
-        warn()
-        return this[newName]
       },
-      set: function (value) {
+      set: value => {
         warn()
-        this[newName] = value
+        o[newName] = value
       }
     })
   }

spec/api-deprecations-spec.js

@@ -1,6 +1,8 @@
+'use strict'
+
 const chai = require('chai')
 const dirtyChai = require('dirty-chai')
-const {deprecations, deprecate, nativeImage} = require('electron')
+const {deprecations, deprecate} = require('electron')
 
 const {expect} = chai
 chai.use(dirtyChai)
@@ -33,26 +35,6 @@ describe('deprecations', () => {
     expect(deprecations.getHandler()).to.be.a('function')
   })
 
-  it('returns a deprecation warning', () => {
-    const messages = []
-
-    deprecations.setHandler(message => {
-      messages.push(message)
-    })
-
-    deprecate.warn('old', 'new')
-    expect(messages).to.deep.equal([`'old' is deprecated. Use 'new' instead.`])
-  })
-
-  it('renames a method', () => {
-    expect(nativeImage.createFromDataUrl).to.be.undefined()
-    expect(nativeImage.createFromDataURL).to.be.a('function')
-
-    deprecate.alias(nativeImage, 'createFromDataUrl', 'createFromDataURL')
-
-    expect(nativeImage.createFromDataUrl).to.be.a('function')
-  })
-
   it('renames a property', () => {
     let msg
     deprecations.setHandler(m => { msg = m })
@@ -80,8 +62,8 @@ describe('deprecations', () => {
     const o = {}
 
     expect(() => {
-      deprecate.removeProperty(o, 'iDontExist')
+      deprecate.removeProperty(o, 'iDoNotExist')
-    }).to.throw(/Cannot deprecate a property on an object which does not have that property/)
+    }).to.throw(/iDoNotExist/)
   })
 
   it('deprecates a property of an object', () => {
@@ -100,6 +82,21 @@ describe('deprecations', () => {
     expect(msg).to.include(prop)
   })
 
+  it('warns only once per item', () => {
+    const messages = []
+    deprecations.setHandler(message => { messages.push(message) })
+
+    const key = 'foo'
+    const val = 'bar'
+    let o = {[key]: val}
+    deprecate.removeProperty(o, key)
+
+    for (let i = 0; i < 3; ++i) {
+      expect(o[key]).to.equal(val)
+      expect(messages).to.have.length(1)
+    }
+  })
+
   it('warns if deprecated property is already set', () => {
     let msg
     deprecations.setHandler(m => { msg = m })