Should be possible to return errors with executeAsyncScript about webdriver HOT 8 CLOSED

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

from webdriver.

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

from webdriver.

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?

from webdriver.

Both options smell of scope creep.

from webdriver.

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.

from webdriver.

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.)

from webdriver.

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.

from webdriver.

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

from webdriver.