puppeteer-sharp icon indicating copy to clipboard operation
puppeteer-sharp copied to clipboard

[BREAKING] Implement Interfaces for public API

Open amaitland opened this issue 2 years ago • 5 comments

Issue #1925

  • Extracts interfaces for the public API classes
  • There are multiple commits to make things slightly easier to review, each commit is a self contained set of changes. (Builds on the previous commits, so you cannot cherry pick them out of order though).
  • ExecutionContext.EvaluateExpressionHandleAsync/EvaluateFunctionHandleAsync are now public and included in the interface. This reduces the number of internal casts to use those methods. They are already publicly accessible via the Frame, so not actually introducing functionality there.
  • ElementHandle.ClickablePointAsync is now public, it's now part of the puppeteer API and simplifies the refactoring. A new Point struct was added.

I was originally going to switch all the classes over to using <inheritdoc/> as I went, that ended up creating too much noise in the commit so I've only done that for the first two commits. I think that should happen after this is finished, reduce the noise in the PR making it easier to review (it's always going to be a lot as is).

There's the question of obsolete methods, I've left most of them out of the interface. Is there a plan to drop them at some point? Otherwise I'll add them.

Making as draft for now, running tests locally now.

amaitland avatar May 19 '22 00:05 amaitland

Tests are passing locally, not clear the failure is related to this PR.

amaitland avatar May 20 '22 00:05 amaitland

I’m not on my computer this week. Give me a few days 🙂

kblok avatar May 20 '22 00:05 kblok

I was originally going to switch all the classes over to using as I went, that ended up creating too much noise in the commit so I've only done that for the first two commits. I think that should happen after this is finished, reduce the noise in the PR making it easier to review (it's always going to be a lot as is).

Let's make this change

kblok avatar May 23 '22 19:05 kblok

Let's make this change

Let's finish the code review first.

amaitland avatar May 23 '22 23:05 amaitland

Any update on this? It would be nice to use these interfaces :)

brnbs avatar Jul 04 '22 14:07 brnbs

Hey guys Any news on the topic?

pthomaz avatar Aug 29 '22 22:08 pthomaz

Merge conflicts have been resolved. Please review when time permits.

I won't be de-duplicating the xml doc in this PR. If someone wants to help you are welcome to submit a PR against my branch or it will need to happen after this PR has been merged.

If there is a plan to merge this PR then before any major changes take place would be preferable as resolving the merge conflicts has been very time consuming. If there's no plan to go forward with this then please close.

amaitland avatar Sep 19 '22 04:09 amaitland