Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Browser/BrowserContext API:s to return Promise #44

Merged
merged 1 commit into from
May 23, 2024

Conversation

allansson
Copy link

Please fill in this template.

Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Thanks a lot! I'm not sure how to review this, though (too many changes! 🙄). So, I trust you 👍

Copy link

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So all the tests are ran concurrently now (at least the ones that work with promise), but within an async function which is awaited on.

Looks good 👍

@inancgumus
Copy link
Member

@ankur22

all the tests are ran concurrently now (at least the ones that work with promise), but within an async function which is awaited on.

How did you get that?

@allansson
Copy link
Author

Wow! Thanks a lot! I'm not sure how to review this, though (too many changes! 🙄). So, I trust you 👍

@inancgumus Oh, I see.

The reason for the amount of changes is that I wrapped all of the test-cases in an async function, because I had to do const page = await browser.newPage() and top-level await is not supported. Every line was changed because wrapping it in a function added indentation on every line.

Otherwise it's just updating the ExpectType to expect a Promise for the affected API:s.

I have run the tests and they all pass. 🙂

@inancgumus
Copy link
Member

inancgumus commented May 23, 2024

@allansson

Got it 👍 Makes sense now. I'd suggest doing each step in a separate commit with a description so that we can easily see and review the changes. For example:

  • Adding the async updates only for Browser
  • Adding the async updates only for BrowserContext
  • Wrapping up things in an async function (with a description "why")

@allansson
Copy link
Author

So all the tests are ran concurrently now (at least the ones that work with promise), but within an async function which is awaited on.

Looks good 👍

@ankur22 The function is actually never executed, it's just linted using the typescript type checker. So it doesn't matter if you await or not, unless you want to continue testing on some return value (e.g. await context.newPage()). 🙂

@allansson allansson merged commit 9c5917f into master-async-browser May 23, 2024
1 check passed
@allansson allansson deleted the async-browser-context branch May 23, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants