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

fix(core): websocket client when running in Bun #34546

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DonIsaac
Copy link

What This PR Does

Fixes a problem where chromium.connectOverCDP fails to connect when running Playwright in Bun. Related issue: oven-sh/bun#9911

The Problem

Bun's HTTP client does not currently support upgrading connections to other protocols, which breaks breaks ws (and other manual websocket implementations). To keep packages using it from breaking, Bun intercepts ws imports and provides a polyfill that wraps a WATWG WebSocket.

This normally works fine, but Playwright bundles ws at build time, so the import is never made. This PR updates utilsBundle.ts to detect if Playwright is running in Bun and, if so, use Bun's built-in ws module.

Notes

Notably, this PR does not shim ws' WebSocketServer, Recevier, or Sender classes.

  • Bun's HTTP server implementation supports upgrade requests, so no corrective actions is needed
  • The latter two are only used by the android backend and operate directly on a socket. This problem is isolated to node:http, so anything built on top of node:net should be fine.

@DonIsaac DonIsaac linked an issue Jan 30, 2025 that may be closed by this pull request

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

24 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-output.spec.ts:80:5 › should show console messages for test @macos-latest-node18-1
⚠️ [playwright-test] › tests/ui-mode-trace.spec.ts:341:5 › should work behind reverse proxy @macos-latest-node18-1
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:72:3 › should remove cookies by name regex @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsercontext-pages.spec.ts:105:3 › should return bounding box with page scale @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/browsertype-launch.spec.ts:22:3 › should reject all promises when browser is closed @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/defaultbrowsercontext-2.spec.ts:28:3 › should work in persistent context @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › by default › loopback address @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:44:14 › page screenshot › should work with a mobile viewport @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:55:14 › page screenshot › should work with a mobile viewport and clip @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:66:14 › page screenshot › should work with a mobile viewport and fullPage @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:206:14 › element screenshot › element screenshot should work with a mobile viewport @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:278:14 › element screenshot › should restore viewport after page screenshot and exception @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/screenshot.spec.ts:291:14 › element screenshot › should restore viewport after page screenshot and timeout @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:224:1 › should complain about newer version of trace in old viewer @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/trace-viewer.spec.ts:1120:1 › should display waitForLoadState even if did not wait for it @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/tracing.spec.ts:785:5 › should not emit after w/o before @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/video.spec.ts:441:5 › screencast › should work for popups @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:380:5 › page screenshot › path option should detect jpeg @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:868:5 › page screenshot animations › should wait for fonts to load @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:245:3 › should upload large file with relative path @webkit-ubuntu-22.04-node18

37713 passed, 654 skipped
✔️✔️✔️

Merge workflow run.

@pavelfeldman
Copy link
Member

I'd suggest that we wait for Bun to support connection upgrade instead, what is this blocking?

@DonIsaac
Copy link
Author

This is blocking users connecting to already-running chromium browsers when running playwright on Bun.

Bun has no plans to support upgrading in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Playwright connectOverCDP() not working
2 participants