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
Conversation
4ae0b27
to
feeda28
Compare
7de2451
to
25ab6bd
Compare
b8fc5a5
to
b7df504
Compare
b7df504
to
451738f
Compare
void OnOpenExternalFinished(const v8::Global<v8::Context>& context, | ||
scoped_refptr<atom::util::Promise> promise, | ||
const std::string& error) { | ||
v8::Isolate* isolate = promise->isolate(); |
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.
Should this be done in the promise helper to reduce this boilerplate being duplicated for all promises?
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 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.
Release Notes Persisted
|
* feat: split openExternal into sync and async * v8::Locker => mate::Locker * fix: enter js env when resolving promise
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])
intoshell.openExternalSync(url[, options])
andshell.openExternal(url[, options])
.shell.openExternal(url[, options])
now returns a promise.This PR also adds specs for previously-untested
shell.openExternal()
and also forshell.openExternalSync()
./cc @miniak @ckerr
Checklist
npm test
passesRelease Notes
Notes: promisify
shell.openExternal()
by splitting it into a sync and async method