-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add replay record command to launch and record #286
Add replay record command to launch and record #286
Conversation
flake
|
...process.env, | ||
RECORD_ALL_CONTENT: "1", |
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.
I think you still need RECORD_ALL_CONTENT
for the record option, right?
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.
It moved up two lines
@@ -830,15 +832,15 @@ async function launchBrowser( | |||
}; | |||
|
|||
const proc = spawn(execPath, browserArgs[browserName], { | |||
detached: !attach, | |||
detached: !opts?.attach, |
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 does detached do?
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.
@jazzdan added it so the CLI process wouldn't exit until the the browser exited. Not sure why (but not really relevant to this PR either)
) { | ||
try { | ||
debug("Options", opts); | ||
|
||
const browser = fuzzyBrowserName(opts.browser) || "chromium"; | ||
assertValidBrowserName(browser); | ||
|
||
const attach = opts.attach || false; | ||
await launchBrowser(browser, [url || "about:blank"], false, { ...opts, verbose: true }); |
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.
Correct me if I'm wrong but this is a breaking change. Do you know if users rely on this command and if we need to have any migrate path?
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.
It is a breaking change. I don't think anyone outside of us and QA is using it though.
Adds
replay record
command to launch and record and updatereplay launch
to only launch the browser. Both now can be manually overridden by settingRECORD_ALL_CONTENT
in the env.