Skip to content

Conversation

pgodwin
Copy link
Contributor

@pgodwin pgodwin commented Dec 11, 2020

Fixes: Stack Overflow issue https://stackoverflow.com/questions/65229445/evaulatescriptaspromiseasync-and-async-await

Summary:

  • Added async to the GetPromiseHandlerScript innerImmediatelyInvokedFuncExpression function. This allows for await to be used in methods passed to this.

Changes: [specify the structures changed]

  • Modified GetPromiseHandlerScript to use an async function
  • Added tests for async/await usage to OffscreenBrowserBasicFacts. Tests pass.

How Has This Been Tested?

  • Added tests Unit tests to OffscreenBrowserBasicFacts.cs, specifically returning values, as well as using fetch to make an AJAX request.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@amaitland amaitland added this to the 87.0.x milestone Dec 11, 2020
@amaitland
Copy link
Member

  • Added async to the GetPromiseHandlerScript innerImmediatelyInvokedFuncExpression function. This allows for await to be used in methods passed to this.

I really had no idea what you mean by your previous comments, much clearer now 👍

If all tests pass then happy to merge this.

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

@pgodwin Your Queensland health email address appears in the commit message (likely part of one of the commits). Do you want to rewrite the commit with a GitHub no reply email?

@pgodwin
Copy link
Contributor Author

pgodwin commented Dec 11, 2020

  • Added async to the GetPromiseHandlerScript innerImmediatelyInvokedFuncExpression function. This allows for await to be used in methods passed to this.

I really had no idea what you mean by your previous comments, much clearer now 👍

If all tests pass then happy to merge this.

Sorry about that. I originally thought it might have been best having the async function being optional via an overload to EvaluateScriptAsPromiseAsync, eg :

public static Task<JavascriptResponse> EvaluateScriptAsPromiseAsync(this IWebBrowser chromiumWebBrowser, string script, TimeSpan? timeout = null, bool useAsync=false)

But after testing it didn't seem to hurt anything having it there, thus this PR.

@pgodwin Your Queensland health email address appears in the commit message (likely part of one of the commits). Do you want to rewrite the commit with a GitHub no reply email?

Not a big deal, it can stay in.

@amaitland amaitland merged commit b8a3641 into cefsharp:master Dec 11, 2020
@amaitland
Copy link
Member

But after testing it didn't seem to hurt anything having it there, thus this PR.

The specific according to MDN says an async function can contain zero or more await expressions, so I think it's technically permissible. It'll likely create an extra promise (not sure if V8 will optimise it away, wouldn't be surprised). Under the circumstances the added flexibility is worth a small increase in overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants