Should be possible to return errors with executeAsyncScript

[andreastt]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28060

jleyba:

The callback provided to the executeAsyncScript function only accepts a single argument that is always treated as a successful completion. It should be possible for users to call this function with an error to indicate their script failed.

Node.js has popularized the "Error-first" callback approach: errors are passed as the first argument, successful values the second.

Another option would be to standardize on the Error-type. If the callback is invoked with an instanceof Error, the script is marked as a failure.

[andreastt]

jleyba:

Here's another option:

Change executeScript to recognize Promise[1] return values and eliminate executeAsyncScript as a command.

If executeScript returns a promise, wait for it to settle. If the promise is resolved, use the resolved value as the command result. If the promise is rejected, return the reason with the command failure.

If the promise doesn't resolve before the script timeout expires, fail the command.

If there is a page unload event before the promise is resolved, fail the command (covering existing behavior of executeAsyncScript).

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

[shs96c]

If we standardise on returning an error if the callback is called with an instance of Error I think this could be level 1. @jgraham, @andreastt, @jleyba: thoughts?

[jleyba]

I think executeScript should be updated to handle promise return values. Then the executeAsyncScript command wouldn't be needed at all

[andreastt]

Both options smell of scope creep.

[jleyba]

Can you elaborate on why you think this is scope creep?

With the current definition, user scripts can only "succeed". Every user will have to implement their own mechanism for reporting errors. Not only are promises the standard way to handle async operations in JS now (see fetch API, service workers, async/await, etc), but adding support to executeScript is trivial for any WebDriver implementation that already supports executeAsyncScript:

// Assumptions
//    userScript - the string function body provided by the user
//    userArgs - argument array provided by the user
//    onSuccess - callback to send a successful response to the driver
//    onFailure - callback to send an error response to the driver

function doExecuteScript(userScript, userArgs) {
  new Promise((resolve, reject) => {
    try {
      let fn = new Function(userScript);
      resolve(fn(...userArgs));
    } catch (ex) {
      reject(ex);
    }
  }).then(onSuccess, onFailure);
}

Also easy for older browsers that don't support promises natively (IE):

function doExecuteScript(userScript, userArgs) {
  try {
    let fn = new Function(userScript);
    let result = fn(...userArgs);
    if (result && typeof result === 'object' && typeof result.then === 'function') {
      result.then(onSuccess, onFailure);
    } else {
      onSuccess(result);
    }
    resolve(fn(...userArgs));
  } catch (ex) {
    onFailure(ex);
  }
}

executeAsyncScript itself can even be implemented in terms of promises:

function doExecuteAsyncScript(userScript, userArgs) {
  new Promise(resolve => {
    userArgs.push(resolve);
    new Function(userScript)(...userArgs);
  }).then(onSuccess, onFailure);
}

Switching to promises would standardize on async JS idioms and allow us to simplify the WebDriver API by eliminating the executeAsyncScript command.

[andreastt]

Don’t get me wrong, I don’t disagree that what you propose is desirable. It is a good proposal.

Can you elaborate on why you think this is scope creep?

It is a breaking change at a very late point in the process as it involves removing the Execute Async Script command and would change the input, output, and expected behaviour of Execute Script. It is scope creep because today is the invisible line the WG set to finish the spec.

(I disagree that it’s worthwhile publishing this spec in W3C with the CR process as it’s unclear both how we can move it forward in the future with changes like this and how we would do maintenance when we find bugs.)

[shs96c]

After discussing on the IRC channel, @AutomatedTester and I agreed that the Promise-based approach is the best way forward, and that landing this mid-way through CR would count as a breaking change.

@andreastt confirmed that this would be relatively easy to implement in geckodriver, so didn't see it as blocking progress on implementing the spec.

We'll wait for a PR on this issue, and then move to CR.

[andreastt]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1335472 to track implementation in Firefox.