Support JavaScript Playwright CLI paths - #67
Conversation
ftes
left a comment
There was a problem hiding this comment.
Danila, thanks for looking into this!
A few thoughts:
I'm not sure we should limit ourselves to node from PATH.
On Unix, we could keep supporting the shebang.
Or we could look at what the official playwright clients do.
Reference: Official Playwright clients
Explicit interpreter everywhere. Do not rely on shebang, but bundle matching Node executable and allow overriding through PLAYWRIGHT_NODEJS_PATH. No dependency on arbitrary node from PATH.
# https://github.com/microsoft/playwright-python/blob/main/playwright/_impl/_driver.py#L28
if sys.platform == "win32":
return (
os.getenv("PLAYWRIGHT_NODEJS_PATH", str(driver_path / "node.exe")),
cli_path,
)
return (os.getenv("PLAYWRIGHT_NODEJS_PATH", str(driver_path / "node")), cli_path)| defp executable_command(executable, args) do | ||
| if String.downcase(Path.extname(executable)) == ".js" do | ||
| node = System.find_executable("node") || raise "Node.js executable not found on PATH" | ||
| {node, [executable | args]} | ||
| else | ||
| {executable, args} | ||
| end | ||
| end |
There was a problem hiding this comment.
Restrict node resolution to Windows only?
Honor shebang on Unix.
| defp executable_command(executable, args) do | |
| if String.downcase(Path.extname(executable)) == ".js" do | |
| node = System.find_executable("node") || raise "Node.js executable not found on PATH" | |
| {node, [executable | args]} | |
| else | |
| {executable, args} | |
| end | |
| end | |
| defp executable_command(executable, args) do | |
| windows? = match?({:win32, _}, :os.type()) | |
| javascript? = String.downcase(Path.extname(executable)) == ".js" | |
| if windows? and javascript? do | |
| node = System.find_executable("node") || raise "Node.js executable not found on PATH" | |
| {node, [executable | args]} | |
| else | |
| {executable, args} | |
| end | |
| end |
|
|
||
| alias PlaywrightEx.PortTransport | ||
|
|
||
| test "launches JavaScript CLI files through Node" do |
There was a problem hiding this comment.
I don't think this unit test is worth the complexity it adds.
If anything, we should expand the test matrix to also run on Windows.
Out of scope for this PR.
|
Thanks, good point. I’ve updated the PR to honor PlaywrightEx doesn’t currently bundle Node, so the explicit override matches the official clients’ convention. Focused tests pass on Linux and Windows. |
| defp check_version(executable) do | ||
| {"Version " <> version, 0} = executable |> Path.expand() |> System.cmd(~w(--version)) | ||
| defp check_version(executable, env) do | ||
| {command, args} = executable_command(Path.expand(executable), ["--version"], env) |
There was a problem hiding this comment.
Why Path.expand executable here, but not in init?
Summary
run-driverThis allows configurations such as
assets/node_modules/playwright/cli.jsto work on Windows, where JavaScript files cannot be passed directly toPort.open/2as executables.Testing
mix format --check-formattedmix test test/playwright_ex/processes/port_transport_test.exs— 1 passedplaywright/cli.jsmix credo --strictreports two pre-existing line-length findings inconnection.exandsupervisor.ex; this change adds no Credo findings.