Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add to deprecations spec #11117

Merged
merged 4 commits into from Nov 20, 2017
Merged

Add to deprecations spec #11117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d05a1f8
first pass at deprecation spec updates
codebytere Nov 14, 2017
99d35f7
a few more updates to tests
codebytere Nov 14, 2017
dc410ef
rename and fix assoc. test
codebytere Nov 14, 2017
7904be8
comment out unused methods
codebytere Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
131 changes: 63 additions & 68 deletions lib/common/api/deprecate.js
View file
Open in desktop
@@ -1,7 +1,6 @@
// Deprecate a method.
const deprecate = function (oldName, newName, fn) {
var warned
warned = false
let warned = false
return function () {
if (!(warned || process.noDeprecation)) {
warned = true
Expand All @@ -11,97 +10,93 @@ const deprecate = function (oldName, newName, fn) {
}
}

// The method is renamed.
deprecate.rename = function (object, oldName, newName) {
var newMethod, warned
warned = false
newMethod = function () {
// The method is aliases and the old method is retained for backwards compat
deprecate.alias = function (object, deprecatedName, existingName) {
let warned = false
const newMethod = function () {
if (!(warned || process.noDeprecation)) {
warned = true
deprecate.warn(oldName, newName)
deprecate.warn(deprecatedName, existingName)
}
return this[newName].apply(this, arguments)
return this[existingName].apply(this, arguments)
}
if (typeof object === 'function') {
object.prototype[oldName] = newMethod
object.prototype[deprecatedName] = newMethod
} else {
object[oldName] = newMethod
}
}

// Forward the method to member.
deprecate.member = function (object, method, member) {
var warned
warned = false
object.prototype[method] = function () {
if (!(warned || process.noDeprecation)) {
warned = true
deprecate.warn(method, member + '.' + method)
}
return this[member][method].apply(this[member], arguments)
object[deprecatedName] = newMethod
}
}

// Deprecate a property.
deprecate.property = function (object, property, method) {
return Object.defineProperty(object, property, {
get: function () {
var warned
warned = false
if (!(warned || process.noDeprecation)) {
warned = true
deprecate.warn(property + ' property', method + ' method')
}
return this[method]()
}
})
}

// Deprecate an event.
deprecate.event = function (emitter, oldName, newName, fn) {
var warned = false
return emitter.on(newName, function (...args) {
// there is listeners for old API.
if (this.listenerCount(oldName) > 0) {
if (!(warned || process.noDeprecation)) {
warned = true
deprecate.warn("'" + oldName + "' event", "'" + newName + "' event")
}
if (fn != null) {
fn.apply(this, arguments)
} else {
this.emit.apply(this, [oldName].concat(args))
}
}
})
}

// Print deprecation warning.
deprecate.warn = function (oldName, newName) {
return deprecate.log(oldName + ' is deprecated. Use ' + newName + ' instead.')
deprecate.warn = (oldName, newName) => {
return deprecate.log(`'${oldName}' is deprecated. Use '${newName}' instead.`)
}

var deprecationHandler = null
let deprecationHandler = null

// Print deprecation message.
deprecate.log = function (message) {
deprecate.log = (message) => {
if (typeof deprecationHandler === 'function') {
deprecationHandler(message)
} else if (process.throwDeprecation) {
throw new Error(message)
} else if (process.traceDeprecation) {
return console.trace(message)
} else {
return console.warn('(electron) ' + message)
return console.warn(`(electron) ${message}`)
}
}

deprecate.setHandler = function (handler) {
deprecate.setHandler = (handler) => {
deprecationHandler = handler
}

deprecate.getHandler = function () {
return deprecationHandler
}
deprecate.getHandler = () => deprecationHandler

// None of the below methods are used, and so will be commented
// out until such time that they are needed to be used and tested.

// // Forward the method to member.
// deprecate.member = (object, method, member) => {
// let warned = false
// object.prototype[method] = function () {
// if (!(warned || process.noDeprecation)) {
// warned = true
// deprecate.warn(method, `${member}.${method}`)
// }
// return this[member][method].apply(this[member], arguments)
// }
// }
//
// // Deprecate a property.
// deprecate.property = (object, property, method) => {
// return Object.defineProperty(object, property, {
// get: function () {
// let warned = false
// if (!(warned || process.noDeprecation)) {
// warned = true
// deprecate.warn(`${property} property`, `${method} method`)
// }
// return this[method]()
// }
// })
// }
//
// // Deprecate an event.
// deprecate.event = (emitter, oldName, newName, fn) => {
// let warned = false
// return emitter.on(newName, function (...args) {
// if (this.listenerCount(oldName) > 0) {
// if (!(warned || process.noDeprecation)) {
// warned = true
// deprecate.warn(`'${oldName}' event`, `'${newName}' event`)
// }
// if (fn != null) {
// fn.apply(this, arguments)
// } else {
// this.emit.apply(this, [oldName].concat(args))
// }
// }
// })
// }

module.exports = deprecate
33 changes: 32 additions & 1 deletion spec/api-deprecations-spec.js
View file
Open in desktop
@@ -1,5 +1,5 @@
const assert = require('assert')
const {deprecations, deprecate} = require('electron')
const {deprecations, deprecate, nativeImage} = require('electron')

describe('deprecations', () => {
beforeEach(() => {
Expand All @@ -18,6 +18,37 @@ describe('deprecations', () => {
assert.deepEqual(messages, ['this is deprecated'])
})

it('returns a deprecation handler after one is set', () => {
const messages = []

deprecations.setHandler((message) => {
messages.push(message)
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

messages is unused here and the deprecate.log() call is unrelated to the test. Looks like these are a copy-paste leftover; all that's really needed for this test is something like
deprecations.setHandler(message => {})


deprecate.log('this is deprecated')
assert(typeof deprecations.getHandler() === 'function')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.deepEqual() here

})

it('returns a deprecation warning', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it('invokes the handler when set')

const messages = []

deprecations.setHandler((message) => {
messages.push(message)
})

deprecate.warn('old', 'new')
assert.deepEqual(messages, [`'old' is deprecated. Use 'new' instead.`])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK but a little brittle, it'll fail if we ever change the warning format. What would you think about something like

let warnings = 0
deprecations.setHandler(message => ++warnings)
deprecate.warn('old', 'new')
assert.equal(warnings, 1)

})

it('renames a method', () => {
assert.equal(typeof nativeImage.createFromDataUrl, 'undefined')
assert.equal(typeof nativeImage.createFromDataURL, 'function')

deprecate.alias(nativeImage, 'createFromDataUrl', 'createFromDataURL')

assert.equal(typeof nativeImage.createFromDataUrl, 'function')
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start but, thinking it over a little, I think this is not a good test of the alias feature. We'd be better off confirming that warnings happen and that deprecatedMethod is routed to existingMethod:

  1. Create a local warned bool set to false
  2. Create a local `called' bool set to false
  3. Call deprecations.setHandler(message => warned = true)
  4. Create a toy object with a method that sets called to true
  5. Call deprecate.alias(toy ... to create an alias for the method
  6. Call the aliased method on the object
  7. assert that called is true
  8. assert that warned is true


it('throws an exception if no deprecation handler is specified', () => {
assert.throws(() => {
deprecate.log('this is deprecated')
Expand Down