feat: Adds binaryArgs to pass additional flags to Firefox binary #1563

denar90 · 2019-03-15T22:57:11Z

Hi,
Can we add one more option to run method?

Motivation: puppeteer/puppeteer#4162
Partly solves #1122

Example of usage

    const CDPPort = 6006
    await webExt.cmd.run({
      sourceDir: extPath,
      firefox: puppeteerFirefox.executablePath(),
      binaryArgs: [
        `-juggler=${CDPPort}`
      ]
    }, {
      shouldExitProgram: false,
    })

    const browserWSEndpoint = `ws://127.0.0.1:${CDPPort}`
    browser = await puppeteerFirefox.connect({
      browserWSEndpoint,
    })

Thanks.

coveralls · 2019-03-15T23:10:14Z

Coverage remained the same at 100.0% when pulling 22753b7 on denar90:binaryArgs into 7311faa on mozilla:master.

rpl

Hi @denar90, thanks for opening this PR, the new feature propose sounds reasonable to me, but it looks that this PR needs some additional changes to be complete, e.g.:

we should add the new option to the ExtensionRunnerParams flow type definition in src/extension-runners/base.js
make sure that the FirefoxAndroidExtensionRunner class is going to print a warning message if the new option is being specified while running the firefox-android target, as it would be unused in that case
expose the feature as a new optional CLI option for the web-ext run command, by adding its definition here
Add some additional test unit, to cover the changes and/or additions applied in this PR, e.g. the following unit test files are likely going to need some small additions (e.g. a new test case, or new assertions):
- tests/unit/test-cmd/test.run.js
- tests/unit/test-extension-runners/test.firefox-desktop.js
- tests/unit/test-extension-runners/test.firefox-android.js (it should be enough to test the new warning here)

Feel free to mention me in a comment if you need help (or if you have any doubts that I can help you with).

rpl · 2019-03-19T15:59:47Z

.gitignore

@@ -1,5 +1,6 @@
 # OSX
 .DS_Store
+.idea


This change seems unrelated.

denar90 · 2019-03-19T22:07:19Z

@rpl thank you so much for the review 👍
I've update PR. Only thing I'm not sure about tests needed. Please, take a look and specify if more cases need to be covered :)

Thanks :)

denar90 · 2019-03-19T22:10:55Z

src/extension-runners/firefox-android.js

@@ -290,6 +291,12 @@ export class FirefoxAndroidExtensionRunner {
        'Firefox for Android target does not support --start-url option.'
      );
    }
+
+    if (this.params.binaryArgs) {


@rpl I was thinking about smth like

const ignoredParams = { profilePath: '--profile-path', keepProfileChanges: '--keep-profile-changes', browserConsole: '--browser-console', preInstall: '--pre-install', startUrl: '--start-url', binaryArgs: '--binary-args', }; const getIgnoredParamsWarningsMessage = (optionName) => { return `Firefox for Android target does not support ${optionName} option`; };

so function printIgnoredParamsWarnings can be a bit cleaner

printIgnoredParamsWarnings() { Object.entries(ignoredParams).forEach((ignoredParam) => { if (this.params[ignoredParam]) { log.warn( getIgnoredParamsWarningsMessage(ignoredParams[ignoredParam]) ); } }); }

Only thing I stuck is flow with some errors, but I can dig in if you approve this approach.

👍 I like the idea

(let me know if you get stuck on issues with flow errors, I'll be happy to help you to figure out how to properly handle those)

denar90 · 2019-03-19T22:12:40Z

btw, should I exclude package-lock.json changes from the PR?

rpl · 2019-03-21T15:24:49Z

btw, should I exclude package-lock.json changes from the PR?

yes please (I briefly looked to the diff and it seems that it is changing the exact versions into non exact versions, and so let's omit it from this PR, thanks for pointing it out!)

rpl · 2019-03-21T15:28:04Z

src/extension-runners/firefox-android.js

@@ -261,6 +261,7 @@ export class FirefoxAndroidExtensionRunner {
  }

  printIgnoredParamsWarnings() {
+


Nit, let's remove this empty line.

rpl · 2019-03-21T15:30:15Z

src/extension-runners/firefox-desktop.js

    } = this.params;

-    const binaryArgs = [];
-
    if (browserConsole) {
      binaryArgs.push('-jsconsole');


uhm... I'm thinking that it may be better to append the additional arguments that the user may be passing to a web-ext run command (instead of prepending them before the ones added internally by web-ext itself).

@Rob--W what do you think about it?

Appending rather than prepending sounds good to me.

rpl · 2019-03-21T15:34:08Z

src/program.js

@@ -503,6 +503,12 @@ Example: $0 --help run.
        demandOption: false,
        type: 'boolean',
      },
+      'binary-args': {
+        describe: 'Browser binary options. More at - ' +
+        'https://developer.mozilla.org/en-US/docs/Mozilla/Command_Line_Options',


Nit, let's indent this line as the other "multi-line" describe values in this file.

wdyt about adding eslint ignore for this line? URL is too long and linter doesn't like it.

@denar90 How about omitting the link and give it a more generic description? e.g. something like the following:

'binary-args': { describe: "Additional command line options passed to the Browser binary', ... }

In my opinion, it may be a reasonable choice, e.g. as part of #1392 we may use the --binary-args also to pass additional CLI options to Chromium browsers.

Otherwise we may opt to include a shortened link instead (e.g. you can use https://mzl.la/2CEvSV5 which point to the above link).

How about omitting the link and give it a more generic description?

SGTM 👍

rpl · 2019-03-21T15:34:56Z

tests/unit/test-extension-runners/test.firefox-desktop.js

@@ -202,6 +202,14 @@ describe('util/extension-runners/firefox-desktop', () => {
    ]);
  });

+  it('passes binaryArgs tp Firefox', async () => {


Nit, typo: tp -> to

whoops 😊

denar90 · 2019-03-21T21:30:17Z

yes please (I briefly looked to the diff and it seems that it is changing the exact versions into non exact versions, and so let's omit it from this PR, thanks for pointing it out!)

had to do force-push, hope it isn't a problem :)

Rob--W · 2019-03-28T14:49:59Z

src/program.js

@@ -503,6 +503,11 @@ Example: $0 --help run.
        demandOption: false,
        type: 'boolean',
      },
+      'binary-args': {


Using binary-args here implies that the command line becomes --binary-args. That is a bit too long to my taste. Could you rename this to args
and add an alias: ['arg'],

I would also have liked the 'a' shorthand, but unfortunately the letter is already taken by the --artifacts-dir command.

Rob--W · 2019-03-28T14:55:29Z

src/extension-runners/firefox-desktop.js

    } = this.params;

-    const binaryArgs = [];
-
    if (browserConsole) {
      binaryArgs.push('-jsconsole');


Appending rather than prepending sounds good to me.

Rob--W · 2019-03-28T14:56:25Z

src/extension-runners/firefox-android.js

+};
+
+const getIgnoredParamsWarningsMessage = (optionName) => {
+  return `Firefox for Android target does not support ${optionName} option`;


While you're at it, let's improve the English:

The Firefox for Android target does not support the ${optionName} option
or
The Firefox for Android target does not support ${optionName}.

Rob--W · 2019-03-28T21:12:01Z

src/extension-runners/firefox-desktop.js

    } = this.params;

-    const binaryArgs = [];


In this version of the PR the arguments are still prepended instead of appended. Restore the binaryArgs variable and use binaryArgs.push(...args); before passing it to firefoxApp.run.

Oh, looks like I misunderstood you. Does it look fine now - 28cb262?

Rob--W · 2019-03-28T21:36:09Z

src/firefox/index.js

@@ -88,7 +88,7 @@ export type FirefoxRunnerParams = {|
  'no-remote'?: boolean,
  'foreground'?: boolean,
  'listen': number,
-  'binary-args'?: Array<string> | string,
+  'args'?: Array<string> | string,


You cannot simply rename binary-args to args here, because this is the API to an external library: https://github.com/mozilla-jetpack/node-fx-runner/blob/71e3848b6adac1054829391c826fd88cfb28f621/lib/run.js#L42-L48

Revert all changes in this file (and use binaryArgs instead of args: binaryArgs in the other file).

Ok, so just keep args only for cli? all other code will remain as is?

Yes. The external interface of web-ext run will use args, and binary-args is only used internally, imposed by the contract of the fx-runner interface.

This reverts commit 14af123.

Rob--W · 2019-03-29T09:07:23Z

tests/unit/test-extension-runners/test.firefox-desktop.js

@@ -172,36 +172,44 @@ describe('util/extension-runners/firefox-desktop', () => {
      params.firefoxApp.run,
      sinon.match.any,
      sinon.match.has(
-        'binaryArgs',
-        sinon.match.array.deepEquals(expectedBinaryArgs)
+        'args',


This one should also be binaryArgs.

Rob--W

Nice. Thanks for taking the efforts to minimize the diff.

rpl · 2019-03-29T21:46:47Z

@denar90 Thanks! Looks great to me too, Let's merge it!

caitmuenster · 2019-03-29T22:04:33Z

Thanks, @denar90! Your contribution has been added to our contribution wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

denar90 · 2019-03-30T21:40:43Z

@rpl @Rob--W thank you for the review and your comments. Really aprriciate your effort 👍

Thanks, @denar90! Your contribution has been added to our contribution wiki.

@caitmuenster thanks 😄

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Wow, I'll be more than happy to become part of the community 🙌
Here's my profile - https://mozillians.org/en-US/u/denar90/

denar90 · 2019-03-31T20:02:46Z

btw, about documentation for the new option, do you need some help with it to make release v3.1 faster?

Rob--W · 2019-03-31T20:48:30Z

@denar90 The documentation that you added at --help should be sufficient.

The list of open tasks for 3.1.0 is at https://github.com/mozilla/web-ext/projects/3

caitmuenster · 2019-04-01T15:09:43Z

Thanks, @denar90! You're vouched.

Welcome onboard! 🎉

denar90 · 2019-04-01T20:19:54Z

The list of open tasks for 3.1.0 is at https://github.com/mozilla/web-ext/projects/3

@Rob--W thank you for the info 👍

@caitmuenster thank you for vouching. I'm really honored 😊

zhl463 · 2019-04-03T06:03:55Z

@denar90 I use this to launch firefox with extension, but shows
Error
connect ECONNREFUSED 127.0.0.1:6006
I can launch firefox, and load extension.

denar90 · 2019-04-03T07:05:12Z

@zhl463 thanks for trying this one :)
Can you follow a guide for puppeteer I've made and give feedback if it works for you?

Rob--W · 2019-04-03T10:11:46Z

@zhl463 That is likely this bug: #1509

zhl463 · 2019-04-08T03:21:06Z

@zhl463 thanks for trying this one :)
Can you follow a guide for puppeteer I've made and give feedback if it works for you?

Yes, I had followed this guide for puppeteer-firefox . but having this error

zhl463 · 2019-04-08T03:22:33Z

@zhl463 That is likely this bug: #1509

Thanks for replying. I will try it.

feat: adds binaryArgs

420b395

denar90 force-pushed the binaryArgs branch from 9ee835b to 420b395 Compare March 16, 2019 20:23

rpl requested changes Mar 19, 2019

View reviewed changes

.gitignore Outdated

@@ -1,5 +1,6 @@

# OSX

.DS_Store

.idea

Copy link

Member

rpl Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated.

denar90 commented Mar 19, 2019

View reviewed changes

rpl changed the title ~~Adds binaryArgs to pass additional flags to Firefox binary~~ feature: Adds binaryArgs to pass additional flags to Firefox binary Mar 21, 2019

rpl requested changes Mar 21, 2019

View reviewed changes

rpl changed the title ~~feature: Adds binaryArgs to pass additional flags to Firefox binary~~ feat: Adds binaryArgs to pass additional flags to Firefox binary Mar 21, 2019

denar90 added 3 commits March 21, 2019 22:51

feat: binaryArgs tests and cli

f6cd95f

refactor: ignoredParams logging for firefox-android

7a8d346

fix: typo in test suite

0076326

denar90 force-pushed the binaryArgs branch from bb8b7c1 to 0076326 Compare March 21, 2019 21:24

fix: binary-args description

baa8cab

Rob--W reviewed Mar 28, 2019

View reviewed changes

denar90 added 2 commits March 28, 2019 22:53

refactor: Rename binaryArgs to args

14af123

refactor: Improve spelling for warning message

117553b

Rob--W reviewed Mar 28, 2019

View reviewed changes

refactor: Append binaryArgs instead of prepend

28cb262

Rob--W reviewed Mar 28, 2019

View reviewed changes

Revert "refactor: Rename binaryArgs to args"

41a85b1

This reverts commit 14af123.

Rob--W reviewed Mar 29, 2019

View reviewed changes

refactor: use args naming for passing additional browser binary options

22753b7

Rob--W approved these changes Mar 29, 2019

View reviewed changes

rpl approved these changes Mar 29, 2019

View reviewed changes

rpl merged commit 44114ee into mozilla:master Mar 29, 2019

rpl added needs: docs type: feature labels Mar 29, 2019

rpl added this to In progress in 3.1.0 via automation Mar 29, 2019

denar90 deleted the binaryArgs branch March 31, 2019 20:02

rpl moved this from In progress to Done in 3.1.0 Jul 1, 2019

		@@ -261,6 +261,7 @@ export class FirefoxAndroidExtensionRunner {
		}

		printIgnoredParamsWarnings() {

feat: Adds binaryArgs to pass additional flags to Firefox binary #1563

feat: Adds binaryArgs to pass additional flags to Firefox binary #1563

Conversation

denar90 commented Mar 15, 2019

coveralls commented Mar 15, 2019 • edited

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denar90 commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denar90 commented Mar 19, 2019

rpl commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denar90 commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

rpl commented Mar 29, 2019

caitmuenster commented Mar 29, 2019

denar90 commented Mar 30, 2019

denar90 commented Mar 31, 2019

Rob--W commented Mar 31, 2019

caitmuenster commented Apr 1, 2019

denar90 commented Apr 1, 2019

zhl463 commented Apr 3, 2019

denar90 commented Apr 3, 2019

Rob--W commented Apr 3, 2019

zhl463 commented Apr 8, 2019

zhl463 commented Apr 8, 2019

coveralls commented Mar 15, 2019 •

edited