Support for executing asynchronous JavaScript

[atifaziz]

This is the PR for #10.

It adds AsyncScript to Browser and Boss. I called the method AsyncScript and not ScriptAsync to highlight that the script has to be asynchronous instead of the execution of the method itself being asynchronous. One would also expect that a method called ScriptAsync would return Task<dynamic> and that's not the case here. What's more, AsyncScript lines up nicely with ExecuteAsyncScript from IJavaScriptExecutor. The one downside of this, however, is that ScriptAsync doesn't sit there next to Script in the IntelliSense pop-up.

I also added Browser.ScriptTimeout as a set-only property since there is no way to read out the value from the driver today (unless I missed something). We could make it readable nonetheless by caching the last value set through the property so that it can be returned later in the getter method. We would also have to assume a default of 0, which is what it is today but it could mislead if it changes in the future. To avoid assuming anything, an option would be for ScriptTimeout be a TimeSpan? that starts out as null, meaning no value was set via the property. Finally, if a set-only property seems odd then we could just turn it into a method. Both would be the most accurate representation of the what's really there today.

Since script timeout is 0 by default in the driver, the experience isn't very nice because the first attempt to use AsyncScript fails. You have to set the timeout first via Browser.ScriptTimeout. That seems against the spirit of the project, which is to make the interface super simple. So my question is, should AsyncScript set the timeout to some reasonable default of 30 seconds if the timeout has never been set before?

Let me know your thoughts on the above points.

[atifaziz]

@StephenCleary Any thoughts?

[StephenCleary]

Sorry, it's been a crazy week! I'll try to take a look at this tonight.

[atifaziz]

Sorry, it's been a crazy week!

Hey, I understand as I have those too. 😄 In fact, this past one was just that.

[StephenCleary]

I agree fully with the naming decision. The IntelliSense ordering is unfortunate, but it is still the best option.

Set-only properties and unintuitive "method pairs" are not great. What do you think about adding a timeout parameter (with a reasonable default) and always having AsyncScript set the timeout first? Since AsyncScript blocks, I think that would make sense.

[atifaziz]

What do you think about adding a timeout parameter (with a reasonable default)

A parameter would be odd because it would continue to remain in effect after the method returns. Since we can't read the timeout, we can't restore it to the previous value after the method is done executing.

"method pairs" are not great

Sorry, but what do you mean by method pairs?

I agree AsyncScript ought to use a reasonable default for the timeout so it doesn't blow up on first use. If we do that, then we can make Browser.ScriptTimeout read-write. AsyncScript will then set the current timeout before calling into IJavaScriptExecutor.ExecuteAsyncScript. What do you think?

[StephenCleary]

A parameter would be odd because it would continue to remain in effect after the method returns.

True, but if they don't drop to the Selenium layer, they won't ever see that effect. Each time they call the method, the parameter is re-set.

Sorry, but what do you mean by method pairs?

Like the user sees AsyncScript and has to know that SetScriptTimeout is related to the behavior of that method.

A read/write property would be better than a write-only property or a method pair, but it could get out of sync with the Selenium layer. Selenium's API was designed around unit testing, and I think it makes sense in BrowserBoss to have an API that is designed around scripting.

[atifaziz]

but it could get out of sync with the Selenium layer

Then again, that only matters for someone who wants to drop to the Selenium layer, right? Like the parameter, the property's value would be used to set the timeout before each call into IJavaScriptExecutor.ExecuteAsyncScript via AsyncScript.

Perhaps I'm trying too hard to keep parity with Session.Timeout; happy to refactor to a parameter if that's the right call for you.

[atifaziz]

Right now, AsyncScript and Script have identical signatures:

public static dynamic AsyncScript(string script, params object[] args)

I'm guessing for the parameter version, we'd add an overload with the timeout parameter coming first? Keeping script and args together will read best and nothing come after args anyhow Therefore it would be:

public static dynamic AsyncScript(TimeSpan timeout, string script, params object[] args)

[atifaziz]

FYI, I just discovered while working on another project that Selenium 3.1 introduces a read-write property called AsynchronousJavaScript on ITimeouts and SetScriptTimeout is now obsolete. See SeleniumHQ/selenium@e4c0a4d for change details. This is good news for a simpler interface and in light of which, I wonder if we should review the decision about the timeout being a parameter of AsyncScript or a property on Browser. 🤔

[StephenCleary]

I believe that the Promise-enabled synchronous script execution makes the asynchronous script execution pretty much obsolete.

What I'd really like to see is a Task<dynamic> for executing scripts truly asynchronously, but that would require Selenium support.