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

Make atom.confirm async #16229

Merged
merged 22 commits into from Jan 9, 2018

Conversation

Projects
None yet
4 participants
@50Wliu
Member

50Wliu commented Nov 18, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This change is a bit more involved than making ContextMenu async. dialog.showMessageBox accepts an optional callback as a third argument that, when supplied, makes the resulting message box non-blocking. Instead of giving the result as a return value, the response is passed to the callback. As such, always passing the callback (even when it didn't exist) would have been a backwards-compatible change. To make Atom support both sync and async versions, I have updated atom.confirm to also accept an optional callback that determines whether or not to use the async version. I also noticed while doing this that Atom's implementation was not fully in line with Electron's implementation and was limited in its functionality, and so the async version corrects this by passing all options directly to Electron. Hopefully in Atom 2.0 we can make async the only supported version. In the meantime, I've updated all of atom/atom's uses of atom.confirm to use the async API and updated the documentation to prefer the async version.

Alternate Designs

The callbackiness is quite messy, especially when we used to return something based on the response value. Anywhere where that occurred I had to pseudo-promisify the dialog and then await the resulting promise. Would it therefore make more sense to directly promisify the async version of dialog.showMessageBox?

Why Should This Be In Core?

atom.applicationDelegate.confirm is implemented in core.

Benefits

Improved UX when a message box appears. See, for example, snow continuing to fall even when a message box appears:
async-message-box

Possible Drawbacks

Unlike making the context menu async, this is an opt-in change. Package authors will have to update their code to support the async message dialog. As such, there is a possibility that people will create issues on atom/atom asking why certain message boxes are async while others aren't. Furthermore, as discussed above, using the callback is much less clean than simply blocking and waiting for a return value.

Applicable Issues

None.

50Wliu added some commits Nov 17, 2017

@UziTech

This comment has been minimized.

Contributor

UziTech commented Nov 30, 2017

👍 +1 on promisifying the async version

callback?()
# Legacy sync version: options can only have `message`,
# `detailedMessage` (optional), and buttons array or object (optional)
{message, detailedMessage, buttons} = options

This comment has been minimized.

@UziTech

UziTech Nov 30, 2017

Contributor

It seems like it would be very unintuitive, and therefore cause difficult to debug issues, if the sync and async versions don't have the same options

This comment has been minimized.

@UziTech

UziTech Nov 30, 2017

Contributor

I would vote for deprecating detailedMessage for detail but keep buttons as an array or object

This comment has been minimized.

@50Wliu

50Wliu Dec 4, 2017

Member

Since the new async version is opt-in, I think that should immediately warn package authors that they need to look at their code and update it accordingly. I explicitly didn't update the sync code path because the goal I had in mind when creating this PR was to completely deprecate and feature-freeze it to give package authors more "incentive" to moving to async.

50Wliu added some commits Dec 4, 2017

@iolsen

This comment has been minimized.

Contributor

iolsen commented Jan 2, 2018

Like the other similar PRs, we're 👍. Merge when ready.

50Wliu added some commits Jan 8, 2018

@50Wliu 50Wliu merged commit cd12345 into master Jan 9, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment