-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
feat: add app.enableSandbox() #14999
Conversation
7002e51
to
05e7361
Compare
Another possible name is |
const appPath = path.join(__dirname, 'fixtures', 'api', 'mixed-sandbox-app') | ||
appProcess = ChildProcess.spawn(remote.process.execPath, [appPath, '--app-enable-sandbox']) | ||
|
||
server.once('error', error => { done(error) }) |
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.
I want to use async/await
, but does it look cleaner here?
// Throws on error.
const client = await Promise.race([
emittedOnce(server, 'connection').then(([, client]) => client),
emittedOnce(server, 'error').then(([, error]) => Promise.reject(error))
])
const data = await emittedOnce(client, 'data')
const argv = JSON.parse(data)
<...>
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.
LGTM
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.
👍 on @alexeykuzmin's suggestion of using async/await in the spec but the rest of the PR looks solid
I can create a separate follow up PR to improve the tests using the await/async pattern. |
Release Notes Persisted
|
Description of Change
Add
app.enableSandbox()
to allow turning on full sandbox without having to run the app with--enable-sandbox
switch.Checklist
npm test
passesRelease Notes
Notes: Added
app.enableSandbox()
API