lino-levan / astral

A high-level puppeteer/playwright-like library for Deno
https://jsr.io/@astral/astral
MIT License
177 stars 7 forks source link

Questions migration from puppeteer to astral #16

Closed lowlighter closed 9 months ago

lowlighter commented 9 months ago

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:

Thanks in advance for your answer πŸ˜„ !

lino-levan commented 9 months ago

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.

lowlighter commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

lowlighter commented 9 months ago

@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