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

Support Puppeteer #201

Open
sandinmyjoints opened this issue Feb 3, 2020 · 7 comments
Open

Support Puppeteer #201

sandinmyjoints opened this issue Feb 3, 2020 · 7 comments
Labels
enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution

Comments

@sandinmyjoints
Copy link

sandinmyjoints commented Feb 3, 2020

Describe the feature you'd like:

Matchers that work when running tests in Puppeteer.

Suggested implementation:

Puppeteer methods return ElementHandles that are proxies for elements in the DOM in the browser it's communicating with. jest-dom currently expects actual HTMLElements. So at a high level, it seems like an implementation approach could be to proxy the methods jest-dom is calling on the HTMLElements into calls to, say, ElementHandle
.evaluate
. ElementHandle's methods are all async, so the matchers would need to be async -- perhaps this could be done with a parallel set of async matchers to the existing (sync) matchers.

Describe alternatives you've considered:

  • Not using jest-dom with puppeteer :(
  • Possibly this would belong in its own repo, something like jest-dom-pptr-adapter

Teachability, Documentation, Adoption, Migration Strategy:

This would likely be straightforward: simply mentioning in the docs that jest-dom is compatible with puppeteer, and noting that the async versions of the matchers would need to be used.

@gnapse
Copy link
Member

gnapse commented Feb 3, 2020

This seems like something that would be good to look into. I have zero experience with puppeteer, but would love to see this tackled and in the process to get to know more about it.

As for the approach to implement it, if it can be handled internally in a way that does not add significant complexity to the existing code, while allowing for this proxying to happen as transparently as possible, then I'm all for it. Alternatively, I'd also be open to this be in a separate repo.

Please, feel free to explore making a PR to add support for this as you see fit, or starting a more technical discussion here of what this would entail, if you're in doubt it would be accepted.

@gnapse gnapse added enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution labels Feb 3, 2020
@gnapse
Copy link
Member

gnapse commented Feb 3, 2020

For instance, an acceptable alternative that seems viable from what you mention the issue is right now, is that we could require all matchers to not consume the received element argument directly, but to pass it via some helper/wrapper function that would detect if we're running in a puppeteer environment, or maybe to check if the element is a puppeteer proxy, and do the puppeteer thing, or else do what we do today.

The thing that makes me wonder if it would be that simple is that you mention something along the lines of matchers being async with puppeteer, so not sure if that poses an additional difficulty in making the same matchers implementation be compatible with both the sync and async way of calling them.

@gnapse
Copy link
Member

gnapse commented Mar 15, 2020

@sandinmyjoints do you have any comments on this? I'm trying to figure out if there can be some actionable in the future, or else close the issue.

@kevin940726
Copy link

kevin940726 commented Dec 6, 2020

I've been playing with this idea recently, and I made a POC of this feature here.

The basic idea is to use sync then-able function to evaluate the element in regular Jest environment, but use ElementHandle.evaluate to evaluate the element in puppeteer environment. The hack is that the matchers can now be both a sync and an async function, so that it can be used seamlessly in both environments.

Here's a simplified version of the code.

function toHaveAttribute(htmlElement, ...args) {
  return evaluate(htmlElement, ...args).then(pass => ({
    pass,
    message: () => "Oops"
  }));
}

The gotcha here is that everything passed to and returned from evaluate should be serializable. Fortunately, I believe this is possible as most of our matchers are dealing with primitive values (or elements, which can also be handled by puppeteer via ElementHandle).

I use typeof htmlElement.evaluate === 'function' there to check if the environment is in puppeteer. This is intentionally vague to achieve a few things:

  1. Not have to depend on puppeteer as a dependency
  2. To support any other environments which can implement this function.

One thing left there is how to handle checkHtmlElement calls. I think it's still possible if we refactor checkHtmlElement and checkHasWindow to return boolean instead of throwing errors. So that we can do some evaluate magic there and throw the error later in Jest environment.

Another option would be to export another function like toHaveAttributeAsync and do the evaluate magic there. The code would be easier to read (can use async/await for instance) and a little less magical. But would require the users to import from another endpoint (maybe @testing-library/jest-dom/puppeteer?).

WDYT? Curious to hear your thoughts :).

@nickserv
Copy link
Member

nickserv commented Dec 8, 2020

Unfortunately if we use the same API and try to hide the async Promises from non-Puppeteer users, we run into issues with @typescript-eslint/require-await which would require await even when not using Puppeteer, and that await could cause different event scheduling and crash tests. I think it would be best to have a separate async API (or possibly a separate package) for tools like Puppeteer. Alternatively we could have a breaking change that makes the entire API async like testing-library/user-event#504, but I'm not sure it would be worth the breakages.

@kevin940726
Copy link

Unfortunately if we use the same API and try to hide the async Promises from non-Puppeteer users, we run into issues with @typescript-eslint/require-await which would require await even when not using Puppeteer, ...

I'm not sure I follow this. How does @typescript-eslint/require-await work? The implementation I showed is returning a thenable, but not a promise, so that in theory, it shouldn't require await. 🤔

@nickserv
Copy link
Member

As far as I understand by looking at the code, @typescript-eslint/require-await does check to see if a type is thenable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed needs discussion We need to discuss this to come up with a good solution
Projects
None yet
Development

No branches or pull requests

4 participants