-
Notifications
You must be signed in to change notification settings - Fork 18
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
Feat support existing ws endpoint #31
Feat support existing ws endpoint #31
Conversation
@@ -96,7 +104,8 @@ export class Browser { | |||
url: "", | |||
}); | |||
const browserWsUrl = new URL(this.#celestial.ws.url); | |||
const wsUrl = `${browserWsUrl.origin}/devtools/page/${targetId}`; | |||
const wsUrl = | |||
`${browserWsUrl.origin}/devtools/page/${targetId}${browserWsUrl.search}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The browserWsUrl.search
is required because some service may use this for authentication
For example browserless.io:
const browser = await puppeteer.connect({
browserWSEndpoint: `wss://chrome.browserless.io?token=******`,
});
Have you tested this out? I'm slightly concerned because we do make |
Yeah I have tested with the following code snippet: import { launch } from "https://esm.sh/gh/lowlighter/astral@feat-support-existing-ws-endpoint/mod.ts";
const browser = await launch({
browserWSEndpoint:
"wss://chrome.browserless.io?token=******",
});
const page = await browser.newPage("http://example.com");
console.log(await page.evaluate(() => document.title));
await browser.close(); And it does work (at least with browserless.io, and an existing local browser as highlighted by the tests):
However as you mentioned calling I'll make a few changes to address this 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test!
Actually, if you could write a section in the docs about "connecting to remote browsers", that would be amazing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Closes #21
This makes it possible to use astral with services that offers remote browsers like https://www.browserless.io/