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.

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

Merged
merged 1 commit into from Oct 10, 2018
Merged

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Oct 5, 2018

Description of Change

Add app.enableSandbox() to allow turning on full sandbox without having to run the app with --enable-sandbox switch.

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
Release Notes

Notes: Added app.enableSandbox() API

@miniak miniak self-assigned this Oct 5, 2018
@miniak miniak requested review from nornagon, tarruda, codebytere, nitsakh and a team October 5, 2018 20:33
@miniak miniak force-pushed the miniak/app-enable-full-sandbox branch from 7002e51 to 05e7361 Compare October 5, 2018 20:36
@miniak
Copy link
Contributor Author

miniak commented Oct 5, 2018

Another possible name is app.enableFullSandbox()

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) })
Copy link
Contributor

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)
<...>

Copy link
Contributor

@tarruda tarruda left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ckerr ckerr left a 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

@miniak
Copy link
Contributor Author

miniak commented Oct 9, 2018

I can create a separate follow up PR to improve the tests using the await/async pattern.

@MarshallOfSound MarshallOfSound merged commit 5bd6de5 into master Oct 10, 2018
@release-clerk
Copy link

release-clerk bot commented Oct 10, 2018

Release Notes Persisted

Added app.enableSandbox() API

@MarshallOfSound MarshallOfSound deleted the miniak/app-enable-full-sandbox branch October 10, 2018 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants