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

Implement BrowserType.connect(wsEndpoint[, options]) #17

Closed
robingustafsson opened this issue Oct 14, 2021 · 4 comments · Fixed by #800
Closed

Implement BrowserType.connect(wsEndpoint[, options]) #17

robingustafsson opened this issue Oct 14, 2021 · 4 comments · Fixed by #800
Assignees
Labels
feature A new feature playwright Issue relating to Playwright compatibility remote remote browser related
Milestone

Comments

@robingustafsson
Copy link
Member

Add support for BrowserType.connect(wsEndpoint[, options]), to connect to an existing browser instance at specified CDP WS endpoint.

Relevant links:

@robingustafsson robingustafsson added feature A new feature playwright Issue relating to Playwright compatibility labels Oct 14, 2021
@robingustafsson robingustafsson changed the title Implement BrowserType.connect(wsEndpoint[, options]) Implement BrowserType.connect(wsEndpoint[, options]) Nov 12, 2021
@robingustafsson robingustafsson added the next Might be eligible for the next planning (not guaranteed!) label Nov 12, 2021
@inancgumus inancgumus self-assigned this Dec 8, 2021
@inancgumus
Copy link
Member

inancgumus commented Dec 21, 2021

It's an excellent next issue to work on because it's good to keep the browser open and try it with updated code while fixing bugs. So I've been researching about this feature, and I have a few things to clarify before I start working on this :) I appreciate your feedback 🙇 ❤️


1st Question

Playwright.Connect method takes a wsEndpoint:

browserType.connect(wsEndpoint[, options])<Promise<Browser>>
    wsEndpoint <string> A browser websocket endpoint to connect to.
    ...

// This method attaches Playwright to an existing browser instance.

So we probably need to change the Connect method in api/browser_type.go from this:

Connect(opts goja.Value)

To this:

Connect(wsEndpoint string, opts goja.Value)

2nd Question

We probably need to add a new method to the module's API like this:

func (m *JSModule) Connect(wsEndpoint string, opts goja.Value) api.Browser {
	// k6common.Throw(m.vu.Runtime(), errors.New("In Progress 🚀"))
	if browserName == "chromium" {
		bt := chromium.NewBrowserType(m.vu.Context())
		return bt.Connect(wsEndpoint, opts)
	}
	...
}

And then call it like this:

import chromium from 'k6/x/browser';

export default function() {
  const browser = chromium.connect("wss://...", { ... });
  const context = browser.newContext();
  const page = context.newPage();
  ...
}

3rd Question

It seems like Playwright's Connect method connects to a Playwright server (IDK what it is 🤷—As far as I see, it might be a way to abstract browsers somehow), but we don't have a server to connect to. They also have a ConnectOverCDP method.

This comment tells the difference:

browserType.connectOverCDP = uses Chrome CDP protocol and works by that only with Chromium-based browsers
browserType.connect = uses Playwright own protocol and works with all Playwright browsers

Since we currently only support Chrome, we will need to adjust the common/Browser type to use the WebSocket URL and give it here (instead of getting it from a browser process). So the rest of the things will be the same:

if err := b.connect(); err != nil {
return nil, err
}

And here:

func (b *Browser) connect() error {
b.logger.Debugf("Browser:connect", "wsURL:%q", b.browserProc.WsURL())
var err error
b.conn, err = NewConnection(b.ctx, b.browserProc.WsURL(), b.logger)

Then, we'll need to skip tracking the following when the Browser value is created by the Connect method:

b.browserProc.didLoseConnection()

Are we going to support closing the browser over CDP? As in here:

// Close shuts down the browser
func (b *Browser) Close() {
b.logger.Debugf("Browser:Close", "")
if !atomic.CompareAndSwapInt64(&b.state, b.state, BrowserStateClosing) {
// If we're already in a closing state then no need to continue.
b.logger.Debugf("Browser:Close", "already in a closing state")
return
}
b.browserProc.GracefulClose()
defer b.browserProc.Terminate()
action := cdpbrowser.Close()
if err := action.Do(cdp.WithExecutor(b.ctx, b.conn)); err != nil {
if _, ok := err.(*websocket.CloseError); !ok {
k6Throw(b.ctx, "unable to execute %T: %v", action, err)
}
}
atomic.CompareAndSwapInt64(&b.state, b.state, BrowserStateClosed)
}

Even if we want to do so, I think we'll need to skip calling these:

b.browserProc.GracefulClose()
defer b.browserProc.Terminate()


In summary, the plan is to give WebSocket URL to a new Browser and let it do the rest.

@robingustafsson
Copy link
Member Author

  1. Yes, the method signature is wrong in the code 🙂
  2. Hm, yeah this is why I wanted to expose the chromium BrowserType:
    //Chromium api.BrowserType
  3. Yes, we don't have a client/server model as Playwright does (to support multiple client-side language SDKs), so only need to support connecting over CDP to a running browser. I think we should break PW compat here an use BrowserType.connect() for connecting over CDP.

@inancgumus inancgumus removed their assignment May 5, 2022
@inancgumus inancgumus removed the next Might be eligible for the next planning (not guaranteed!) label Sep 12, 2022
@abacaj
Copy link

abacaj commented Oct 18, 2022

Would be great to have this as browsers are launched in docker and connect is necessary to control the browser vs launch.

@inancgumus
Copy link
Member

We might want to resurrect this issue if we want to connect to a browser running in a Docker container. Also, the connect method can automatically use this if the extension runs on the Cloud. Moving this to the v0.9.0 milestone until we decide not to include it.

@inancgumus inancgumus added this to the v0.9.0 milestone Feb 2, 2023
@ankur22 ankur22 self-assigned this Feb 10, 2023
@ankur22 ankur22 removed their assignment Feb 20, 2023
@ka3de ka3de self-assigned this Feb 21, 2023
@ka3de ka3de closed this as completed in #800 Mar 2, 2023
@inancgumus inancgumus added the remote remote browser related label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature playwright Issue relating to Playwright compatibility remote remote browser related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants