Closed jrolfs closed 2 years ago
Sorry, I've taken a while to get to reviewing these. I like how you've implemented the LocatorPromise
concept!
I have a couple of ideas that I'll run past you regarding the API.
1.
Do we need the .within()
API, or could we chain without it? Although, a user probably doesn't chain as often as you do in the examples it can seem redundant/noisey. If we didn't need it at all, we'd be less consistent with Testing Library, but be providing a simpler API with one less API. If Screen
is a Page & LocatorQueries
, could we make all methods that used to produce Locator
produce WithinReturn<Locator>
and redefine WithinReturn
to create a Locator
augmented with LocatorQueries
?
I guess the issue I see with this is that Locator
has quite a few methods like .first()
, .last()
, .locator()
and .page()
which can produce non-augmented Locator
or Page
instances. If we started to manually augment each of these, where does it end? We might end up playing whack-a-mole every time Playwright adds a new method trying to consistently augment each of them...
OK, I've convinced myself that being explicit and having a .within()
method is a good idea for now at least... I think my suggestion has the potential to be a good idea but an imperfect implementation or hastily chosen API is dangerous so we would need to tread carefully (unless we have a way of implementing it which is highly maintainable).
Do other's agree/disagree?
2.
I've changed some of my code to use .findByRole(...)
recently and something I've noticed which is quite annoying is that it's quite jarring that it returns Promise<Locator>
.
In practice, you sometimes end up writing code like this:
await screen.findByRole('button', { name: 'Apply' });
await screen.getByRole('button', { name: 'Apply' }).click();
// -- or --
const applyButton = await screen.findByRole('button', { name: 'Apply' });
await applyButton.click();
But wishing that you could continue to chain Locator
methods onto the Promise<Locator>
, like so:
await screen.findByRole('button', { name: 'Apply' }).click();
Could the concept of LocatorPromise
providing a .within()
method be extended to support calling other Locator
methods directly upon the LocatorPromise
or would this cause problems?
@sebinsua heh, once again, your questions/propositions follow my internal dialog/thought process pretty closely...
Supporting chaining without within()
would be possible, but I stuck with within()
both for Testing Library consistency and for reasons similar to the concerns you shared. Adding a single .within()
to the Locator
API felt more... Locator
-y and disambiguates the query methods from the rest of the Locator
methods.
I think my suggestion has the potential to be a good idea but an imperfect implementation or hastily, chosen API is dangerous so we would need to tread carefully (unless, we have a way of implementing it which is highly maintainable)
I might try to see if I can come up with something in TypeScript that would provide exhaustive checking as to whether we've "augmented" every method that returns a Locator
... that might get us pretty close to "a way of implementing it which is highly maintainable," in that compilation would fail until we handled any new Locator
methods in a future Playwright release.
This also occurred to me, but the tricky bit here is that not all of the methods on Locator
return a Promise
. How do we feel about the trade-off of making the handful of synchronous methods on Locator
(locator()
, first()
, etc.) asynchronous in order to preserve laziness (preventing the need for first await
)?
I guess I could probably use the same technique I did with within()
to "propagate" the Promise
"down the chain," but just like the within()
thing above, it might get pretty gnarly in practice. It will probably also add additional cognitive overhead and nuance to the Playwright APIs. With within()
we are at least isolating this concept to query chaining.
Edit: another point for keeping Promise<Locator>
that just occurred to me is that I can see find*
queries often used to wait for a particular piece of the page to be ready before continuing the rest of a test. If we don't require a user to await
at that point in time, then when that waiting happens is less clear. Maybe allowing both is more powerful, but I see it as another potential point of confusion as well.
Let me know what you think about the next steps given my responses above. Do you think we should get this released on beta more or less as is so that we can iterate, or do you think it's worth seeing through these ideas?
@gajus — please do let us know if you have any thoughts here as well :D
Regarding 2., I didn't mean that the findBy*
queries wouldn't return a Promise
, I was thinking that as well as within
there would be methods that chained all other public methods of Locator
, effectively meaning that each would return a Promise
. So, you would still be able to await screen.findByRole('button', { name: 'Apply' })
but could also await screen.findByRole('button', { name: 'Apply' }).click()
.
I don't really understand the implementation of this well enough to comment now, but presumably in some way click
would .then
the LocatorPromise
before calling .click()
against the Locator
. I'd expect the synchronous methods to become asynchronous in this case, however, if we had awaited the findBy*
on its own, I'd expect the methods to be untouched on the Locator
returned as expected.
Ahaaaa, ok that's an interesting idea. It still requires find*
to always be await
-ed, but allows chaining a single interaction onto the LocatorPromise
. I like it, and I don't think the implementation should be too bad. I'll give it a shot soon, thanks.
Just wanted to say that I'm using Playwright Test Library for the first time today, and I would really like to have this :)
We've got a bunch of "page object"-style helper methods, like:
export async function togglePausePane(screen: Screen) {
return screen.queryByRole("button", { name: "motion_photos_paused" }).click();
}
and right now we're passing around screen
everywhere, which we got out of the {page, screen, within}
arg to the test
function.
We aren't passing within
around, and I wasn't able to figure out a way to use the global within
export properly. So, being able to do something like screen.queryByTestId("abcd").within().whatever()
would be great!
@markerikson, that's a compelling reason to get this chaining stuff in there (as if we hadn't already convinced ourselves). I also have a project with quite a few of those sorts of helper functions that currently take a Page
instance.
I apologize for the confusion with within()
— the global export is currently reserved for the legacy ElementHandle
query API. The next major release will consolidate around the new Locator
query API and hopefully resolve the confusion associated with that legacy API.
I'm going to go ahead and merge this as is, @sebinsua. I think we can address the additional ergonomics around LocatorPromise
can as a separate feature/pull request:
But wishing that you could continue to chain Locator methods onto the Promise
, like so: await screen.findByRole('button', { name: 'Apply' }).click();
Could the concept of LocatorPromise providing a .within() method be extended to support calling other Locator methods directly upon the LocatorPromise or would this cause problems?
Rest assured, I still plan on implementing this and adding more inline documentation (and a bit of refactoring) to ensure everything stays maintainable :)
Edit: oh yeah, I also wanted to mention that you can rest assured that the Locator
API will stay pretty stable into 5.0. The only thing I currently plan on changing is the name of the locatorFixture
export which should be a single-line change.
:tada: This PR is included in version 4.5.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
Thanks!
It looks like this got published as part of playwright-testing-library
, but not @playwright-testing-library/test
. Could you bump that package as well?
(tbh I'm not sure I actually understand what the real difference is between those two packages.)
Ugh, lame, sorry about that. I'll have to see why that didn't fail the build.
The biggest difference is that one works with playwright the library and one works with @playwright/test the library and runner combined. Playwright has a build process with which they release many different package "flavors" from the same codebase instead of a more modular set of packages. We, therefore, need to do the same thing to some extent to support both playwright and @playwright/test.
@markerikson, alright, I manually published @playwright-testing-library/test@4.5.0. Sorry about that.
Thanks for the feedback on the confusion around the packages btw — honestly, I'd attribute some of that confusion to Playwright / Playwright Test, but I may try to clarify the difference in the readme further.
@jrolfs thank you! as a fellow lib maintainer, I really appreciate the fast response :)
@markerikson sure thing, let me know if you run into any other issues :)
Implement query chaining per response to @gajus comment https://github.com/testing-library/playwright-testing-library/pull/498#issuecomment-1241427489.
I think this came out to be pretty cool/useful/powerful.
Synchronous
Synchronous + Asynchronous
Todo