feat: add persistent launch #47
Conversation
src/browser.ts
Outdated
| userDataDir: string | null; | ||
| persistent: boolean; | ||
| gracefulShutdown: boolean; |
There was a problem hiding this comment.
I don't really understand what usecase this solves. At most, this PR should introduce the userDataDir (kind of wordy name btw) option.
If the user specifies where they want the data dir to be, they probably want it to be persisted. It'd be weird to care about the location and then not make it persisted.
I think gracefulShutdown should be true 100% of the time. I don't understand why it's a flag to be enabled.
There was a problem hiding this comment.
If the user specifies where they want the data dir to be, they probably want it to be persisted. It'd be weird to care about the location and then not make it persisted.
When I wrote this functionality I was thinking about the possibility of specifying any folder, including its ability to be temporary.
Of course, I could rewrite some of the code and remove the persistent option, and only save the directory when userDataDir is specified.
I think gracefulShutdown should be true 100% of the time. I don't understand why it's a flag to be enabled.
Maybe then add an env check if this built-in functionality should be disabled?
if (Deno.env.get("DISABLE_ASTRAL_GRACEFUL_SHUTDOWN") === undefined) { ... }For example, if you need to implement custom behavior with signals other than the current one and the ability to close all browsers using the closeAllBrowsers method.
There was a problem hiding this comment.
Of course, I could rewrite some of the code and remove the persistent option, and only save the directory when userDataDir is specified.
I would be in favor of that.
Maybe then add an env check if this built-in functionality should be disabled?
I don't think it's necessary. If someone asks for it, we can consider it but I think this over-complicates the API surface.
There was a problem hiding this comment.
I don't think it's necessary. If someone asks for it, we can consider it but I think this over-complicates the API surface.
In my pet project I have implemented waiting for an empty task queue and disconnecting from the database at termination.
The problem is that signals are processed synchronously and do not wait for other listeners, so I need to disable listening inside this implementation and manually close browsers.
There was a problem hiding this comment.
What's the advantage of graceful shutdown? Shouldn't users manually close the browser anyways?
There was a problem hiding this comment.
If you attempt to terminate a process early, graceful shutdown ensures an orderly and controlled termination of an application, allowing it to complete necessary tasks, release resources, and maintain system stability.
There was a problem hiding this comment.
I'm sorry for not being super clear here, I don't really see the advantage of adding this much complexity to the API when this would quite literally be like 4 or 5 lines to write if the user really needed it. I think managing the browser instances should be done by the API consumer and not us.
I could be wrong about this, and maybe there's a usecase that you're thinking of that I'm not that requires this functionality built into Astral, but it really seems complicated for something unrelated to what we do.
I do really like the customizability of the userDataDir and would be happy to accept this PR with just those changes.
teawinzero
changed the title
examples/persistent_launch.ts
Outdated
| // Launch the browser | ||
| const browser = await launch({ | ||
| // Setting the directory where the browser will store user-specific data, such as cookies, local storage, and other browsing data. | ||
| userDataDir: resolve("./my-browser-data"), |
There was a problem hiding this comment.
If you do not do this, the directory will be created in the folder where the browser is launched.
There was a problem hiding this comment.
We should probably automatically resolve it (in launch). I feel like making it spawn where the browser is launched would definitely not be behavior people would expect.
src/browser.ts
Outdated
| @@ -66,6 +66,8 @@ async function runCommand( | |||
| export interface BrowserOptions { | |||
| headless: boolean; | |||
| product: "chrome" | "firefox"; | |||
| userDataDir: string | null; | |||
| persistent: boolean; | |||
There was a problem hiding this comment.
persistent seems to be removable with no ill affect.
Co-authored-by: Lino Le Van <[email protected]>
lino-levan
changed the title
|
@teawinzero Are you willing to push this forward? |
|
Closing due to merge conflicts, sorry this couldn't get merged! |
Ctrl + Csignal is received