From 5a35c3a2792eb54a149b883f5f38a45dc8a43d5e Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 25 Jan 2019 14:23:24 -0800 Subject: [PATCH] chore: fix promisify helper (#16544) * chore: fix promise deprecation helper * fix deprecations * update deprecation tests --- lib/browser/api/app.js | 2 +- lib/browser/api/session.js | 10 +++++----- lib/browser/api/web-contents.js | 2 +- lib/common/api/deprecate.js | 16 +++++++-------- lib/renderer/api/desktop-capturer.js | 2 +- spec/api-app-spec.js | 6 +++--- spec/api-browser-window-spec.js | 30 +++++++++++++++------------- spec/api-deprecations-spec.js | 7 +++---- spec/api-desktop-capturer-spec.js | 5 +++-- spec/api-protocol-spec.js | 14 ++++++------- spec/api-session-spec.js | 18 +++++++++++++++-- spec/api-web-contents-spec.js | 1 + 12 files changed, 65 insertions(+), 48 deletions(-) diff --git a/lib/browser/api/app.js b/lib/browser/api/app.js index 2c0695b6b18d..8c51a02ae9a3 100644 --- a/lib/browser/api/app.js +++ b/lib/browser/api/app.js @@ -36,7 +36,7 @@ Object.assign(app, { } }) -app.getFileIcon = deprecate.promisify(app.getFileIcon, 3) +app.getFileIcon = deprecate.promisify(app.getFileIcon) const nativeAppMetrics = app.getAppMetrics app.getAppMetrics = () => { diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index 22ce35c30c8d..96362cc99146 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -11,10 +11,10 @@ const fromPartition = (partition) => { if (!session[wrappedSymbol]) { session[wrappedSymbol] = true const { cookies } = session - cookies.flushStore = deprecate.promisify(cookies.flushStore, 0) - cookies.get = deprecate.promisify(cookies.get, 1) - cookies.remove = deprecate.promisify(cookies.remove, 2) - cookies.set = deprecate.promisify(cookies.set, 1) + cookies.flushStore = deprecate.promisify(cookies.flushStore) + cookies.get = deprecate.promisify(cookies.get) + cookies.remove = deprecate.promisify(cookies.remove) + cookies.set = deprecate.promisify(cookies.set) } return session } @@ -35,6 +35,6 @@ Object.setPrototypeOf(Session.prototype, EventEmitter.prototype) Object.setPrototypeOf(Cookies.prototype, EventEmitter.prototype) Session.prototype._init = function () { - this.protocol.isProtocolHandled = deprecate.promisify(this.protocol.isProtocolHandled, 2) + this.protocol.isProtocolHandled = deprecate.promisify(this.protocol.isProtocolHandled) app.emit('session-created', this) } diff --git a/lib/browser/api/web-contents.js b/lib/browser/api/web-contents.js index 9e36231964b4..87c0c126a5d5 100644 --- a/lib/browser/api/web-contents.js +++ b/lib/browser/api/web-contents.js @@ -374,7 +374,7 @@ WebContents.prototype._init = function () { // render-view-deleted event, so ignore the listeners warning. this.setMaxListeners(0) - this.capturePage = deprecate.promisify(this.capturePage, 2) + this.capturePage = deprecate.promisify(this.capturePage) // Dispatch IPC messages to the ipc module. this.on('-ipc-message', function (event, internal, channel, args) { diff --git a/lib/common/api/deprecate.js b/lib/common/api/deprecate.js index 6f049bbcde81..42218003f78c 100644 --- a/lib/common/api/deprecate.js +++ b/lib/common/api/deprecate.js @@ -69,27 +69,27 @@ const deprecate = { }) }, - promisify: (fn, cbParamIndex) => { + promisify: (fn) => { const fnName = fn.name || 'function' const oldName = `${fnName} with callbacks` const newName = `${fnName} with Promises` const warn = warnOnce(oldName, newName) return function (...params) { - const cb = params.splice(cbParamIndex, 1)[0] + let cb + if (params.length > 0 && typeof params[params.length - 1] === 'function') { + cb = params.pop() + } const promise = fn.apply(this, params) - - if (typeof cb !== 'function') return promise + if (!cb) return promise if (process.enablePromiseAPIs) warn() return promise .then(res => { process.nextTick(() => { - cb(null, res) + cb.length === 2 ? cb(null, res) : cb(res) }) }, err => { - process.nextTick(() => { - cb(err) - }) + process.nextTick(() => cb(err)) }) } }, diff --git a/lib/renderer/api/desktop-capturer.js b/lib/renderer/api/desktop-capturer.js index d33a355c4773..249a998d1fe5 100644 --- a/lib/renderer/api/desktop-capturer.js +++ b/lib/renderer/api/desktop-capturer.js @@ -56,4 +56,4 @@ const getSources = (options) => { }) } -exports.getSources = deprecate.promisify(getSources, 1) +exports.getSources = deprecate.promisify(getSources) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index d6c4e9d973e5..a17d6013a860 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -859,7 +859,7 @@ describe('app module', () => { }) // TODO(codebytere): remove when promisification is complete - it('fetches a non-empty icon (callback)', () => { + it('fetches a non-empty icon (callback)', (done) => { app.getFileIcon(iconPath, (icon) => { expect(icon.isEmpty()).to.be.false() done() @@ -875,7 +875,7 @@ describe('app module', () => { }) // TODO(codebytere): remove when promisification is complete - it('fetches normal icon size by default (callback)', () => { + it('fetches normal icon size by default (callback)', (done) => { app.getFileIcon(iconPath, (icon) => { const size = icon.getSize() @@ -903,7 +903,7 @@ describe('app module', () => { }) // TODO(codebytere): remove when promisification is complete - it('fetches a normal icon (callback)', () => { + it('fetches a normal icon (callback)', (done) => { app.getFileIcon(iconPath, { size: 'normal' }, (icon) => { const size = icon.getSize() diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 1637c32993f3..edd333b25c1f 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -507,7 +507,7 @@ describe('BrowserWindow module', () => { }) // TODO(codebytere): remove when promisification is complete - it('returns a Promise with a Buffer (callback)', () => { + it('returns a Promise with a Buffer (callback)', (done) => { w.capturePage({ x: 0, y: 0, @@ -540,24 +540,26 @@ describe('BrowserWindow module', () => { }) // TODO(codebytere): remove when promisification is complete - it('preserves transparency (callback)', async () => { - const w = await openTheWindow({ + it('preserves transparency (callback)', (done) => { + openTheWindow({ show: false, width: 400, height: 400, transparent: true - }) - const p = emittedOnce(w, 'ready-to-show') - w.loadURL('data:text/html,') - await p - w.show() + }).then(w => { + const p = emittedOnce(w, 'ready-to-show') + w.loadURL('data:text/html,') + p.then(() => { + w.show() - w.capturePage((image) => { - const imgBuffer = image.toPNG() - // Check the 25th byte in the PNG. - // Values can be 0,2,3,4, or 6. We want 6, which is RGB + Alpha - expect(imgBuffer[25]).to.equal(6) - done() + w.capturePage((image) => { + const imgBuffer = image.toPNG() + // Check the 25th byte in the PNG. + // Values can be 0,2,3,4, or 6. We want 6, which is RGB + Alpha + expect(imgBuffer[25]).to.equal(6) + done() + }) + }) }) }) }) diff --git a/spec/api-deprecations-spec.js b/spec/api-deprecations-spec.js index bebbf1f10aea..7236107f262d 100644 --- a/spec/api-deprecations-spec.js +++ b/spec/api-deprecations-spec.js @@ -138,7 +138,7 @@ describe('deprecations', () => { it('acts as a pass-through for promise-based invocations', async () => { enableCallbackWarnings() - promiseFunc = deprecate.promisify(promiseFunc, 1) + promiseFunc = deprecate.promisify(promiseFunc) const actual = await promiseFunc(expected) expect(actual).to.equal(expected) @@ -147,12 +147,11 @@ describe('deprecations', () => { it('warns exactly once for callback-based invocations', (done) => { enableCallbackWarnings() - promiseFunc = deprecate.promisify(promiseFunc, 1) + promiseFunc = deprecate.promisify(promiseFunc) let callbackCount = 0 const invocationCount = 3 - const callback = (error, actual) => { - expect(error).to.be.null() + const callback = (actual) => { expect(actual).to.equal(expected) expect(warnings).to.have.lengthOf(1) expect(warnings[0]).to.include('promiseFunc') diff --git a/spec/api-desktop-capturer-spec.js b/spec/api-desktop-capturer-spec.js index 9d888d646c8d..60d90c881b8f 100644 --- a/spec/api-desktop-capturer-spec.js +++ b/spec/api-desktop-capturer-spec.js @@ -62,9 +62,10 @@ describe('desktopCapturer', () => { // TODO(codebytere): remove when promisification is complete it('responds to subsequent calls of different options (callback)', (done) => { let callCount = 0 - const callback = (error, sources) => { + const callback = (err, sources) => { callCount++ - expect(error).to.be.null() + expect(err).to.be.null() + expect(sources).to.not.be.null() if (callCount === 2) done() } diff --git a/spec/api-protocol-spec.js b/spec/api-protocol-spec.js index f78b6235deb0..50b600ca4ae5 100644 --- a/spec/api-protocol-spec.js +++ b/spec/api-protocol-spec.js @@ -680,14 +680,14 @@ describe('protocol module', () => { }) }) - describe('protocol.isProtocolHandled', (done) => { + describe('protocol.isProtocolHandled', () => { it('returns true for about:', async () => { const result = await protocol.isProtocolHandled('about') assert.strictEqual(result, true) }) // TODO(codebytere): remove when promisification is complete - it('returns true for about: (callback)', () => { + it('returns true for about: (callback)', (done) => { protocol.isProtocolHandled('about', (result) => { assert.strictEqual(result, true) done() @@ -700,7 +700,7 @@ describe('protocol module', () => { }) // TODO(codebytere): remove when promisification is complete - it('returns true for file: (callback)', () => { + it('returns true for file: (callback)', (done) => { protocol.isProtocolHandled('file', (result) => { assert.strictEqual(result, true) done() @@ -722,7 +722,7 @@ describe('protocol module', () => { assert.strictEqual(result, false) }) - it('returns true for custom protocol', () => { + it('returns true for custom protocol', (done) => { const emptyHandler = (request, callback) => callback() protocol.registerStringProtocol(protocolName, emptyHandler, async (error) => { assert.strictEqual(error, null) @@ -733,7 +733,7 @@ describe('protocol module', () => { }) // TODO(codebytere): remove when promisification is complete - it('returns true for custom protocol (callback)', () => { + it('returns true for custom protocol (callback)', (done) => { const emptyHandler = (request, callback) => callback() protocol.registerStringProtocol(protocolName, emptyHandler, (error) => { assert.strictEqual(error, null) @@ -744,7 +744,7 @@ describe('protocol module', () => { }) }) - it('returns true for intercepted protocol', () => { + it('returns true for intercepted protocol', (done) => { const emptyHandler = (request, callback) => callback() protocol.interceptStringProtocol('http', emptyHandler, async (error) => { assert.strictEqual(error, null) @@ -755,7 +755,7 @@ describe('protocol module', () => { }) // TODO(codebytere): remove when promisification is complete - it('returns true for intercepted protocol (callback)', () => { + it('returns true for intercepted protocol (callback)', (done) => { const emptyHandler = (request, callback) => callback() protocol.interceptStringProtocol('http', emptyHandler, (error) => { assert.strictEqual(error, null) diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 8a0611275333..22fb42d17d16 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -266,12 +266,13 @@ describe('session module', () => { } expect(error).to.be.undefined(error) }) + it('with callbacks', (done) => { const name = 'foo' const value = 'bar' const { cookies } = session.defaultSession - cookies.set({ url, name, value }, error => { + cookies.set({ url, name, value }, (error) => { if (error) return done(error) cookies.flushStore(error => done(error)) }) @@ -279,6 +280,19 @@ describe('session module', () => { }) }) + describe('ses.cookies.flushStore(callback)', () => { + it('flushes the cookies to disk and invokes the callback when done', (done) => { + session.defaultSession.cookies.set({ + url: url, + name: 'foo', + value: 'bar' + }, (error) => { + if (error) return done(error) + session.defaultSession.cookies.flushStore(() => done()) + }) + }) + }) + it('should survive an app restart for persistent partition', async () => { const appPath = path.join(__dirname, 'fixtures', 'api', 'cookie-app') const electronPath = remote.getGlobal('process').execPath @@ -735,7 +749,7 @@ describe('session module', () => { }) }) - describe('ses.setCertificateVerifyProc(callback)', () => { + describe('ses.setCertificateVerifyProc(callback)', (done) => { let server = null beforeEach((done) => { diff --git a/spec/api-web-contents-spec.js b/spec/api-web-contents-spec.js index c744a3db59dc..84730ceb007b 100644 --- a/spec/api-web-contents-spec.js +++ b/spec/api-web-contents-spec.js @@ -597,6 +597,7 @@ describe('webContents module', () => { } }) + // TODO(codebytere): remove when promisification is complete it('can set the correct zoom level (callback)', async () => { try { await w.loadURL('about:blank')