-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(webkit): allow running WebKit via WSL on Windows #36358
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
base: main
Are you sure you want to change the base?
Conversation
10a64dd
to
34667d2
Compare
This comment has been minimized.
This comment has been minimized.
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.
This is very clean. I like it.
socket.on('error', reject); | ||
}); | ||
|
||
const [executable, ...args] = process.argv.slice(2); |
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.
Even though this is run by us, this should have proper error handling.
env: { | ||
...this.amendEnvironment(env, userDataDir, executable, browserArguments, options.channel), | ||
"WSLENV": "SOCKET_ADDRESS", | ||
'SOCKET_ADDRESS': (transportServer?.address() as any)?.port?.toString() ?? '', |
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.
What is our goal here if we don't have a transport?
bae0c10
to
18c9ba8
Compare
ba3bd42
to
48251d4
Compare
48251d4
to
62bd7f5
Compare
override amendEnvironment(env: Env, userDataDir: string, executable: string, browserArguments: string[], channel?: string): Env { | ||
return { | ||
...env, | ||
// CURL_COOKIE_JAR_PATH: path.join(channel === 'webkit-wsl' ? translatePathToWSL(userDataDir) : userDataDir, 'cookiejar.db'), |
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.
if condition.
62bd7f5
to
e8c3971
Compare
} | ||
|
||
export type LaunchLifecycleHooks = { | ||
preLaunch(): Promise<void>; |
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.
This could be an abstract method on BrowserType
.
} | ||
|
||
export type LaunchLifecycleHooks = { | ||
preLaunch(): Promise<void>; | ||
onExit(): Promise<void>; |
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.
Probably an abstract method as well.
readPipe(): NodeJS.ReadableStream; | ||
writePipe(): NodeJS.WritableStream; | ||
rewriteArgs(args: string[]): string[]; | ||
rewriteExecutable(): string; |
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.
An abstract method.
readPipe(): NodeJS.ReadableStream; | ||
writePipe(): NodeJS.WritableStream; |
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.
For these, perhaps we can do createTransport()
that will combine these pipes and waitForReadyState()
together.
rewriteExecutable: () => 'wsl', | ||
rewriteArgs: (args: string[]) => { | ||
const executablePath = registry.findExecutable('webkit-wsl')!.executablePathOrDie('node'); | ||
return [ | ||
'-d', | ||
'playwright', | ||
'--cd', | ||
'/home/pwuser', | ||
'/home/pwuser/node/bin/node', | ||
'/home/pwuser/webkit-wsl-pipe-wrapper.mjs', | ||
executablePath, | ||
...args, | ||
]; | ||
}, | ||
}; |
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.
Can we replace this with something similar to pw_run.sh
?
71966c1
to
ba2acea
Compare
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"8 failed 18 flaky3753 passed, 430 skipped Merge workflow run. |
No description provided.