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

Questions migration from puppeteer to astral #16

Closed
lowlighter opened this issue Oct 9, 2023 · 7 comments
Closed

Questions migration from puppeteer to astral #16

lowlighter opened this issue Oct 9, 2023 · 7 comments
Labels
enhancement New feature or request question Further information is requested

Comments

@lowlighter
Copy link
Contributor

Hi !
I'm looking to possibly use this as a replacement of npm:puppeteer as the later isn't designed for deno which seems to cause some troubles on windows (SIGHUP unsupported, instances not properly closing) and during unit testing (leaking ops)

So I'm considering switching to astral, as the others puppeteer-like libs are either outdated or have some caveats from what I tested.

I have a few questions and possibly features requests (if this is in the project scope) though:

  • How reliable is it for unit testing ? Currently I need to pass { sanitizeResources: false, sanitizeOps: false } with puppeteer but the aim is to be able to have proper testing without any leaking ops (including windows).
  • Would it be possible to avoid the use of Deno.command for the browser installation ? On windows requiring the access of Powershell which creates a big attack vector since you need to give a shell access. I haven't experimented it but it seems that it's possible to use DecompressionStream to unzip the archive natively (ref: https://medium.com/deno-the-complete-reference/zip-and-unzip-files-in-deno-ee282da7369f)
  • Similarly, would it be possible to be able to pass the cache path in getBinary() so it's possible to use the same cache as the app using it and have more controls about deno permissions ?
  • There are some methods that doesn't seems to have any equivalent in astral currently:
  • How do you achieve page.on("console", (message: { text: () => string }) => console.debug("puppeteer: ${message.text()}")) and page.on("pageerror", (error: { message: string }) => console.warn("puppeteer: ${error.message}")) on astral to log console message from the browser to your app ? Is it through page.addEventListener
  • What's the equivalent of omitBackground for screenshot (unless it's enabled by default) ?

Thanks in advance for your answer 😄 !

@lino-levan
Copy link
Owner

How reliable is it for unit testing ?

One of my core design principles is RELIABILITY. I've always been frustrated with puppeteer and my hope with astral is to make it as reliable as playwright (if not more so).

Would it be possible to avoid the use of Deno.command for the browser installation?

I'd be willing to accept a PR to do this, though I'm slightly concerned about the performance implication of doing it in JS land. I don't care too much since most of the time installing the browser is spent downloading and not unzipping the file. If you're not willing to make a PR, I could look into it.

Similarly, would it be possible to be able to pass the cache path in getBinary() so it's possible to use the same cache as the app using it and have more controls about deno permissions ?

I'm not quite sure what you mean by this. Could you clarify? Are you asking for the ability to change the cache directory?

Page.setContent()
Page.setViewport()

Definitely on the priority list, I've been meaning to add them.

Puppeteer.connect()

This will most likely be an argument to launch. I've been waiting for someone to ask for this, it's a pretty trivial addition.

How do you achieve ... on astral to log console message from the browser to your app?

Right now it would require using the raw bindings, but I've been meaning to add more events. I'm curious what your usecase is. Is it just to log the page console? This shouldn't be too hard to add.

What's the equivalent of omitBackground for screenshot (unless it's enabled by default) ?

There isn't one currently, but I was really curious because it doesn't seem to exist as an option in the CDP protocol. After some investigation, I found out why.
image
Essentially, puppeteer just makes the background transparent for the screenshot and then undoes that. I'm not sure if this is the best way of doing that (it seems super race-y). I'm starting to wonder whether or not I should just expose the transparency emulation directly? Curious to hear your thoughts here.

Thanks for the comments and interest in astral! You make a lot of good points regarding how the library should move forward. I'm excited to hear back from you.

@lino-levan lino-levan added enhancement New feature or request question Further information is requested labels Oct 9, 2023
@lowlighter
Copy link
Contributor Author

lowlighter commented Oct 9, 2023

I'd be willing to accept a PR to do this, though I'm slightly concerned about the performance implication of doing it in JS land. I don't care too much since most of the time installing the browser is spent downloading and not unzipping the file. If you're not willing to make a PR, I could look into it.

I've been poc-ing it on github codespace and it doesn't look too bad actually, I'll try to open a pr 👍

I'm not quite sure what you mean by this. Could you clarify? Are you asking for the ability to change the cache directory?

Yeah basically. From my understanding currently it's written in $HOME/.astral, but I'd like to be able to change to $PWD/.astral for example. I understand the common path allow to avoid re-download, but in my case I'd lik to just pass --allow-read=. and --allow-write=. to make a "self-contained app" that doesn't explore outside of its boundaries

Page.setContent()
Definitely on the priority list, I've been meaning to add them.

I have a PR almost ready for it if you want for this one 👍
I didn't look into the viewport one though, I couldn't find where to change this looking at the bindings

This will most likely be an argument to launch. I've been waiting for someone to ask for this, it's a pretty trivial addition.

Ok nice 🙂
Waiting for this !

Right now it would require using the raw bindings, but I've been meaning to add more events. I'm curious what your usecase is. Is it just to log the page console? This shouldn't be too hard to add.

Yeah basically it's mostly to be able to forward the browser log to deno logs for easier debugging

Essentially, puppeteer just makes the background transparent for the screenshot and then undoes that. I'm not sure if this is the best way of doing that (it seems super race-y). I'm starting to wonder whether or not I should just expose the transparency emulation directly? Curious to hear your thoughts here.

Personally I don't mind if as a user you need to set the background transparent by yourself, as what they've done seems indeed a bit hacky. So if they were a method to change the background color/transparacy before taking a screenshot I'd be fine with it

Thanks for your quick response !

@lino-levan
Copy link
Owner

I've been poc-ing it on github codespace and it doesn't look too bad actually, I'll try to open a pr 👍

Great! Excited to review.

Yeah basically. From my understanding currently it's written in $HOME/.astral, but I'd like to be able to change to $PWD/.astral for example.

Seems reasonable. This makes sense to me.

I have a PR almost ready for it if you want for this one 👍

That would be great!

I didn't look into the viewport one though, I couldn't find where to change this looking at the bindings

That one is definitely a bit trickier, I have some ideas on how to tackle it though.

Yeah basically it's mostly to be able to forward the browser log to deno logs for easier debugging

Should this be the default (or maybe just an option to newPage or launch)?

Personally I don't mind if as a user you need to set the background transparent by yourself, as what they've done seems indeed a bit hacky. So if they were a method to change the background color/transparacy before taking a screenshot I'd be fine with it

I'll look into exposing it somehow.

Thanks for your interest!

@lowlighter
Copy link
Contributor Author

That one is definitely a bit trickier, I have some ideas on how to tackle it though.

Nice !

Should this be the default (or maybe just an option to newPage or launch)?

I don't think this should be the default, because you may end up being flooded by logs, lots of website don't actually clean their debugging log 😅 Plus I don't know if it consumes a lot of resource to attach listeners to these events (though probably not that much)

I guess if you'd like something more deno-ish it could be an API similar to Deno.spawn where you'd pass something similar to {console:"piped"|"inherit"|"null", errors:"throw"|"log"|"null"} where "piped" would be a new stream that the user handle themselve, "inherit" would directly call console.log while "null" would do nothing, and for errors you could be able to "throw" them or just "log" them.

But the callback API from puppeteer works too, it really depends what kind of API you want to offer.

Personally, if the feature is there I'm happy 😆

I'll look into exposing it somehow.

Cool, if all of these get implemented, I think I'll be able to switch to Astro fully on my projects
Thanks a lot for what you've already achieved !


Another request, I noticed that the port 9222 is hardcoded, would it be possible to make it variable ?
The use case would be to be able to run deno test --parallel as it would be possible to run multiple instance of the browsers, but even in the future if multiple apps runs Astral then they wouldn't conflicts

@lino-levan
Copy link
Owner

Another request, I noticed that the port 9222 is hardcoded, would it be possible to make it variable ?

Definitely possible, but I'm not really sure what the best way of automatically picking one is.

The use case would be to be able to run deno test --parallel as it would be possible to run multiple instance of the browsers, but even in the future if multiple apps runs Astral then they wouldn't conflicts

Hm, this may become a problem with us automatically downloading the browser. I assume it'll just crash because multiple processes are trying to write to the same file at the same time.

@lino-levan
Copy link
Owner

Let me know if I missed anything, I opened tracking issues for each of the remaining requests. Thanks for issue!

@lowlighter
Copy link
Contributor Author

@lino-levan Perfect !

I'd say #21 and #24 are the features I'd like the most currently
Then it'd be #20 and #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants