-
Notifications
You must be signed in to change notification settings - Fork 16.2k
fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
const {Buffer} = require('buffer') | ||
const childProcess = require('child_process') | ||
const path = require('path') | ||
const util = require('util') | ||
|
||
const hasProp = {}.hasOwnProperty | ||
|
||
|
@@ -217,6 +218,28 @@ | |
arguments[arg] = newPath | ||
return old.apply(this, arguments) | ||
} | ||
if (old[util.promisify.custom]) { | ||
module[name][util.promisify.custom] = function () { | ||
const p = arguments[arg] | ||
const [isAsar, asarPath, filePath] = splitPath(p) | ||
if (!isAsar) { | ||
return old[util.promisify.custom].apply(this, arguments) | ||
} | ||
|
||
const archive = getOrCreateArchive(asarPath) | ||
if (!archive) { | ||
return new Promise(() => invalidArchiveError(asarPath)) | ||
} | ||
|
||
const newPath = archive.copyFileOut(filePath) | ||
if (!newPath) { | ||
return new Promise(() => notFoundError(asarPath, filePath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MarshallOfSound that won't work, |
||
} | ||
|
||
arguments[arg] = newPath | ||
return old[util.promisify.custom].apply(this, arguments) | ||
} | ||
} | ||
} | ||
|
||
// Override fs APIs. | ||
|
@@ -373,6 +396,18 @@ | |
}) | ||
} | ||
|
||
fs.exists[util.promisify.custom] = function (p) { | ||
const [isAsar, asarPath, filePath] = splitPath(p) | ||
if (!isAsar) { | ||
return exists[util.promisify.custom](p) | ||
} | ||
const archive = getOrCreateArchive(asarPath) | ||
if (!archive) { | ||
return new Promise(() => invalidArchiveError(asarPath)) | ||
} | ||
return Promise.resolve(archive.stat(filePath) !== false) | ||
} | ||
|
||
const {existsSync} = fs | ||
fs.existsSync = function (p) { | ||
const [isAsar, asarPath, filePath] = splitPath(p) | ||
|
@@ -680,18 +715,22 @@ | |
// called by `childProcess.{exec,execSync}`, causing | ||
// Electron to consider the full command as a single path | ||
// to an archive. | ||
['exec', 'execSync'].forEach(function (functionName) { | ||
const old = childProcess[functionName] | ||
childProcess[functionName] = function () { | ||
const {exec, execSync} = childProcess | ||
childProcess.exec = invokeWithNoAsar(exec) | ||
childProcess.exec[util.promisify.custom] = invokeWithNoAsar(exec[util.promisify.custom]) | ||
childProcess.execSync = invokeWithNoAsar(execSync) | ||
|
||
function invokeWithNoAsar (func) { | ||
return function () { | ||
const processNoAsarOriginalValue = process.noAsar | ||
process.noAsar = true | ||
try { | ||
return old.apply(this, arguments) | ||
return func.apply(this, arguments) | ||
} finally { | ||
process.noAsar = processNoAsarOriginalValue | ||
} | ||
} | ||
}) | ||
} | ||
|
||
overrideAPI(fs, 'open') | ||
overrideAPI(childProcess, 'execFile') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ const assert = require('assert') | |
const ChildProcess = require('child_process') | ||
const fs = require('fs') | ||
const path = require('path') | ||
const util = require('util') | ||
const {closeWindow} = require('./window-helpers') | ||
|
||
const nativeImage = require('electron').nativeImage | ||
|
@@ -549,6 +550,60 @@ describe('asar package', function () { | |
}) | ||
}) | ||
|
||
describe('fs.exists', function () { | ||
it('handles an existing file', function (done) { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'file1') | ||
// eslint-disable-next-line | ||
fs.exists(p, function (exists) { | ||
assert.equal(exists, true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MarshallOfSound I don't want that to be back-ported to 2.0. It's small amount of code compared to the rest of the tests. |
||
done() | ||
}) | ||
}) | ||
|
||
it('handles a non-existent file', function (done) { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'not-exist') | ||
// eslint-disable-next-line | ||
fs.exists(p, function (exists) { | ||
assert.equal(exists, false) | ||
done() | ||
}) | ||
}) | ||
|
||
it('promisified version handles an existing file', (done) => { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'file1') | ||
// eslint-disable-next-line | ||
util.promisify(fs.exists)(p).then(exists => { | ||
assert.equal(exists, true) | ||
done() | ||
}) | ||
}) | ||
|
||
it('promisified version handles a non-existent file', function (done) { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'not-exist') | ||
// eslint-disable-next-line | ||
util.promisify(fs.exists)(p).then(exists => { | ||
assert.equal(exists, false) | ||
done() | ||
}) | ||
}) | ||
}) | ||
|
||
describe('fs.existsSync', function () { | ||
it('handles an existing file', function () { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'file1') | ||
assert.doesNotThrow(function () { | ||
assert.equal(fs.existsSync(p), true) | ||
}) | ||
}) | ||
|
||
it('handles a non-existent file', function () { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'not-exist') | ||
assert.doesNotThrow(function () { | ||
assert.equal(fs.existsSync(p), false) | ||
}) | ||
}) | ||
}) | ||
|
||
describe('fs.access', function () { | ||
it('accesses a normal file', function (done) { | ||
var p = path.join(fixtures, 'asar', 'a.asar', 'file1') | ||
|
@@ -644,6 +699,12 @@ describe('asar package', function () { | |
done() | ||
}) | ||
}) | ||
|
||
it('can be promisified', () => { | ||
return util.promisify(ChildProcess.exec)('echo ' + echo + ' foo bar').then(({ stdout }) => { | ||
assert.equal(stdout.toString().replace(/\r/g, ''), echo + ' foo bar\n') | ||
}) | ||
}) | ||
}) | ||
|
||
describe('child_process.execSync', function () { | ||
|
@@ -680,6 +741,12 @@ describe('asar package', function () { | |
var output = execFileSync(echo, ['test']) | ||
assert.equal(String(output), 'test\n') | ||
}) | ||
|
||
it('can be promisified', () => { | ||
return util.promisify(ChildProcess.execFile)(echo, ['test']).then(({ stdout }) => { | ||
assert.equal(stdout, 'test\n') | ||
}) | ||
}) | ||
}) | ||
|
||
describe('internalModuleReadJSON', function () { | ||
|
@@ -700,6 +767,18 @@ describe('asar package', function () { | |
}) | ||
}) | ||
|
||
describe('util.promisify', function () { | ||
it('can promisify all fs functions', function () { | ||
const originalFs = require('original-fs') | ||
|
||
for (const key in originalFs) { | ||
if (originalFs[key][util.promisify.custom] && !fs[key][util.promisify.custom]) { | ||
assert(false, `fs.${key}[util.promisify.custom] missing`) | ||
} | ||
} | ||
}) | ||
}) | ||
|
||
describe('process.noAsar', function () { | ||
var errorName = process.platform === 'win32' ? 'ENOENT' : 'ENOTDIR' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
return Promise.reject(invalidArchiveError(asarPath))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarshallOfSound that won't work,
invalidArchiveError
throws the errorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, this will result in a rejected promise right? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes