feat: promisify executeJavaScript #17312
Conversation
5397848 to
17b9734
Compare
dd1afbd to
78fb556
Compare
e57ccd8 to
00c3c87
Compare
00c3c87 to
8514d97
Compare
8514d97 to
5ee49a0
Compare
5ee49a0 to
be95cef
Compare
be95cef to
93891e8
Compare
|
@miniak just resolve the markdown file conflict and we can get this across the finish line 🏁 |
93891e8 to
cb5be7e
Compare
|
@codebytere resolved, can you please merge after the builds pass again? |
|
Release Notes Persisted
|
|
Though I initially filed a bug (#12159) about the docs discrepancy for webFrame execute APIs, I very much agree with @MarshallOfSound's assessment that this was primarily a docs issue. The webFrame execute APIs have always been synchronous (which makes perfect sense, this is in-process js evaluation), and under the hood they still are, despite now returning promises. The callback used to deliver the result synchronously before the method returned. There is nothing gained and only loss of flexibility when you wrap an actually sync api in a promise. It's trivial for the user to go from sync to promise (Promise.(resolve|reject)) but going from promise to sync is impossible. So this change (for webFrame) is:
Please revert it (for webFrame). An actual improvement (for webFrame) would be to replace the sync callback with a sync return (or add). |
|
If you believe that promise versions of the webFrame execute APIs should exist out the box (though it's not clear to me why they are needed), that can be done without breaking changes or at least loss of valuable sync functionality, by adding new async wrapper methods. |
|
Just to be clear, I am just trying to have a conversation about the impact of the webFrame.execute change, to reach a consensus on how to handle it better, without breaking code or at least losing current capabilities. 6.0 goes into quiet period on the 23rd. To summarize, the webFrame.execute calls are synchronous. Wrapping them in promises is not an enhancement, it takes away the flexibility of using them synchronously, breaking existing code (example in #19161). |
|
cc @miniak ☝️ |
|
@miniak Your comment does not appear here, but I'll respond because I'd like to for this conversation to proceed. My suggestion from #19161:
However if it's simpler and quicker to drop the callback, that would be fine too. |
class SyncScriptExecutionCallback : public blink::WebScriptExecutionCallback {
public:
const char* error_message;
v8::Local<v8::Value> result;
explicit SyncScriptExecutionCallback() : error_message(nullptr) {}
void Completed(
const blink::WebVector<v8::Local<v8::Value>>& result) override {
if (!result.empty()) {
if (!result[0].IsEmpty()) {
// Right now only single results per frame is supported.
this->result = result[0];
} else {
this->error_message =
"Script failed to execute, this normally means an error "
"was thrown. Check the renderer console for the error.";
}
} else {
this->error_message =
"WebFrame was removed before script could run. This normally means "
"the underlying frame was destroyed";
}
}
};
v8::Local<v8::Value> ExecuteJavaScript(mate::Arguments* args,
v8::Local<v8::Value> window,
const base::string16& code) {
bool has_user_gesture = false;
args->GetNext(&has_user_gesture);
SyncScriptExecutionCallback sync_callback;
GetRenderFrame(window)->GetWebFrame()->RequestExecuteScriptAndReturnValue(
blink::WebScriptSource(blink::WebString::FromUTF16(code)),
has_user_gesture, &sync_callback);
if (sync_callback.error_message) {
args->ThrowError(sync_callback.error_message);
return v8::Null(args->isolate());
}
return sync_callback.result;
}Is this acceptable? |
|
@bughit while it may have sometimes worked synchronously, this has always been quite clearly documented as an async callback. A given function performing synchronously doesn’t mean the API contract is synchronous, and as such it is, in our opinion, not a breaking change in that manner. As such, we plan to keep this as it is. |
|
Not sometimes, in practice, which matters much more than incorrect documentation, it has always been synchronous, so it does break code, but more significantly, it can't be worked around. A value can easily be turned to a promise but a fulfilled promise can not synchronously be turned into a value. So the cost of this change is very high, but the benefit is 0. When I initially started using this api, I saw the documentation was incorrect and I reported it (#12159), so then as a fallback, experimentally and by examining the implementation I confirmed that the call is synchronous. But now you are saying that in the absence of valid docs, it was wrong of me to use the api as it was actually implemented (synchronously) and so I only have myself to blame that my code is suddenly broken in 6.0? If you don't want to revert existing methods, can you at least accept sync varieties (webFrame.executeJavaScriptSync)? There is value in those and no harm. |
which creates a
switch (option) {
case kAsynchronousBlockingOnload:
executor->RunAsync(PausableScriptExecutor::kOnloadBlocking);
break;
case kAsynchronous:
executor->RunAsync(PausableScriptExecutor::kNonBlocking);
break;
case kSynchronous:
executor->Run();
break;
}
enum ScriptExecutionType {
// Execute script synchronously, unless the page is suspended.
kSynchronous,
// Execute script asynchronously.
kAsynchronous,
// Execute script asynchronously, blocking the window.onload event.
kAsynchronousBlockingOnload
};I am not a 100% sure but I think suspended means unloaded to save memory, which does not apply to electron, which means |
Description of Change
contents.executeJavaScript()webFrame.executeJavaScript()webFrame.executeJavaScriptInIsolatedWorld()webviewTag.executeJavaScript()Checklist
npm testpassesRelease Notes
Notes: Converted
contents.executeJavaScript(),webFrame.executeJavaScript(),webFrame.executeJavaScriptInIsolatedWorld()andwebviewTag.executeJavaScript()to return Promises instead of taking callbacks.