api: remove waitForLoadState() in favor of PageEvent.page(options)

[dgozman]

No description provided.

[jperl]

This is a useful helper for third party libraries, for example we use it in waitForPage.

The purpose of waitForPage is to have a standard way of looking up pages based on their creation order. We use it to make creating tests with multiple pages cleaner.

let page = await waitForPage(context, 1).

[yury-s]

The purpose of waitForPage is to have a standard way of looking up pages based on their creation order. We use it to make creating tests with multiple pages cleaner.

@jperl IIUC you should still be able to set IndexedPage.createdIndex correctly. 'page' events will fire in order of the page creation, the new API just forces clients to wait for the Page to be initialized and get to a particular state before using it. After the page reached the state you can set the creationIndex:

const creationIndex = ++lastPageCreationIndex;
event.page().then(p => p.creationIndex = creationIndex);

. context.pages()today returns only pages which has already been initialized by playwright but may not have reached the same state as you'd expect inPageEvent.page(). One way to address that non-determinism would be to pass similar options to BrowserContext.pages()`. Would that work for your scenario?

[jperl]

Yes if context.pages allowed passing a LifecycleEvent to filter by context.pages({ waitUntil: "load" }), that would solve my problem nicely.

We want to mark the creationIndex immediately, in case there is a race. And allow users to decide on the waitUntil lifecycle event in waitForPage. That is why page.waitForLoadState was nice, or context.pages({ waitUntil }) would also work.

I could hack around it by marking a bunch of load states on the page inside the "page" event handler, but that is not so clean.

[yury-s]

Another approach that could work without context.pages() is something like this:

const indexedPages = new Set;
const lastPageIndex = 0;
context.on('page', event => {
  const pageIndex = ++lastPageIndex;
  event.page().then(page => {
    page.creationIndex = pageIndex;
    indexedPages.add(page);
  });  
});

Now that we have page event per context and each context starts empty enables this logic which was impossible before.

[jperl]

Yes I already do it this way, the issue is allowing waitForPage to specify which lifecycle event to wait for on that page.

[dgozman]

I find api like waitForPage hard to use.

• For example, if your client calls waitForPage at some random point, it will currently wait for some random navigation to reach load, not the initial navigation (as you probably intended?)
• Also, calling context.pages() will wait until all pages from the context are initialized, and that could happen long after the page you are waiting for has reached the load state (or even, redirected away to somewhere else).

I'd suggest to use PageEvent to assign creation index, and explicitly call event.page(options) with desired waitUntil.

[jperl]

For example, if your client calls waitForPage at some random point, it will currently wait for some random navigation to reach load, not the initial navigation (as you probably intended?)

The point of the waitForPage API is to make generating tests for multiple pages easier. The waitForPage is called right before the first action on that page. Ex:

test("pscp", async () => {
  await page.goto("https://pscp.tv/");
  await page.click("[data-qa=login-twitter]");
  // switch to twitter popup
  page = await qawolf.waitForPage(page.context(), 1, { waitUntil: "load" });
  await page.type("[data-qa=username]", "USERNAME");
  await page.type("[data-qa=password]", "PASSWORD");
  await page.press("[data-qa=submit]", "Enter");
  // switch back to application
  page = await qawolf.waitForPage(page.context(), 0);
  await page.click("[data-qa=logout]");
});

Also, calling context.pages() will wait until all pages from the context are initialized, and that could happen long after the page you are waiting for has reached the load state (or even, redirected away to somewhere else).

Good point, so I would prefer page.waitForLoadState. That being said, I can workaround this.