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: split openExternal into sync and async #16176

Merged
merged 3 commits into from Jan 15, 2019
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Dec 21, 2018

Description of Change

This API can't be promisified in the same way as previous apis, as its callback is optional.

This therefore follows Node's conventions and splits shell.openExternal(url[, options, callback]) into shell.openExternalSync(url[, options]) and shell.openExternal(url[, options]).
shell.openExternal(url[, options]) now returns a promise.

This PR also adds specs for previously-untested shell.openExternal() and also for shell.openExternalSync().

/cc @miniak @ckerr

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • relevant documentation is changed or added
  • PR title follows semantic commit guidelines
  • PR release notes describe the change in a way relevant to app-developers

Release Notes

Notes: promisify shell.openExternal() by splitting it into a sync and async method

@codebytere codebytere requested review from a team December 21, 2018 17:02
docs/api/shell.md Outdated Show resolved Hide resolved
docs/api/shell.md Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the promise-open-external branch 3 times, most recently from 4ae0b27 to feeda28 Compare December 22, 2018 15:58
@codebytere codebytere changed the title [wip] feat: split openExternal into sync and async feat: split openExternal into sync and async Jan 11, 2019
void OnOpenExternalFinished(const v8::Global<v8::Context>& context,
scoped_refptr<atom::util::Promise> promise,
const std::string& error) {
v8::Isolate* isolate = promise->isolate();
Copy link
Member

Choose a reason for hiding this comment

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

Should this be done in the promise helper to reduce this boilerplate being duplicated for all promises?

Copy link
Member

Choose a reason for hiding this comment

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

I agree moving the code to promise helper. But I would prefer doing it in a separate PR since it is a refactoring that modifies multiple files.

@codebytere codebytere merged commit 0881fd6 into master Jan 15, 2019
@release-clerk
Copy link

release-clerk bot commented Jan 15, 2019

Release Notes Persisted

promisify shell.openExternal() by splitting it into a sync and async method

@codebytere codebytere deleted the promise-open-external branch January 15, 2019 04:35
akisctx pushed a commit to akisctx/electron that referenced this pull request Jan 22, 2019
* feat: split openExternal into sync and async

* v8::Locker => mate::Locker

* fix: enter js env when resolving promise
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