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

Execution of web-ext run with chromium defined as target in config file does not launch Chromium #1862

Open
sineway opened this issue Mar 17, 2020 · 6 comments

Comments

@sineway
Copy link

sineway commented Mar 17, 2020

Is this a feature request or a bug?

Bug

What is the current behavior?

Execution of web-ext run with chromium defined as target in config file does not launch Chromium, while web-ext run --target chromium works as expected.

What is the expected or desired behavior?

Launch Chromium

Version information

$ node --version
v13.11.0

$ npm --version
6.14.2

$ web-ext --version
4.0.0
@sineway sineway changed the title Execution of "web-ext run" with "chromium" defined in "run.target" array does not launch Chromium Execution of web-ext run with chromium defined as target in config file does not launch Chromium Mar 17, 2020
@Rob--W
Copy link
Member

Rob--W commented Mar 17, 2020

Could you share the config file that you've used for your test? And did you confirm that it was loaded (that will be shown in the command output)?

@sineway
Copy link
Author

sineway commented Mar 18, 2020

It is simple web-ext-config.js in the project root

module.exports = {
    run: {
        target: [
            "firefox-desktop",
            "chromium"
        ]
    },
    build: {
        overwriteDest: true
    }
};

Yes, it is shown in the command output

$ web-ext run
Applying config file: ./web-ext-config.js

@rpl
Copy link
Member

rpl commented Mar 26, 2020

I suspected that the config was being overridden by the CLI parameters, and running web-ext in verbose mode does confirm that:

...
[program.js][info] Applying config file: ./web-ext-config.js
[config.js][debug] Loading JS config file: "/.../web-ext-config.js" (resolved to "/.../web-ext-config.js")
[config.js][debug] Favoring CLI: target=firefox-desktop over configuration: target=chromium,firefox-desktop

The "Favoring CLI" log is being logged by src/config.js:

web-ext/src/config.js

Lines 100 to 106 in de9211a

if (wasValueSetOnCLI) {
log.debug(
`Favoring CLI: ${option}=${argvFromCLI[option]} over ` +
`configuration: ${option}=${configObject[option]}`);
newArgv[option] = argvFromCLI[option];
continue;
}

But the value from cli is actually the default value for that option:

web-ext/src/program.js

Lines 494 to 502 in de9211a

'target': {
alias: 't',
describe: 'The extensions runners to enable. Specify this option ' +
'multiple times to run against multiple targets.',
default: 'firefox-desktop',
demandOption: false,
type: 'array',
choices: ['firefox-desktop', 'firefox-android', 'chromium'],
},

I verified that "removing the default value from the target cli option configuration" (linked above) does prevent this issue (well, to be precise it does "workaround the issue"), but the root cause is basically the same one behind #1327: the config file is being loaded after yargs has parsed the command line options and applied its own defaults.

And so I think that a more proper way to fix this issue (as well as #1327) would be to make sure that:

  • the config file is discovered and loaded before yargs has parsed the command line option (which would prevent that yargs would fail for missing mandatory parameters missing from the command line because are configured in the config file, which is the underlying reason for "Missing required arguments: api-key, api-secret" with correct configuration #1327), but we would need to also take into account that as part of the CLI options the user may have also specified --config or --no-config-discovery options
  • and be sure that values coming from config are overriding yargs default values for the options not explicitly part of the options on the command file (which is the underlying reason for this issue as described above)

@motin
Copy link

motin commented Jun 26, 2020

For the record, #1327 is fixed in recently released 4.3.0 but this issue remains unresolved.

@Rob--W
Copy link
Member

Rob--W commented Jun 26, 2020

#1327 was fixed by hooking on the validation logic, to defer the validation of required parameters until after reading from the config file.

If we want to fix this bug in a similar way, then we would have to figure out if it is possible to delay the logic that sets the defaults.

motin added a commit to mozilla-extensions/regrets-reporter that referenced this issue Jun 27, 2020
@rpl
Copy link
Member

rpl commented Apr 13, 2021

As a workaround for this issue (until we have been able to handle it with a more proper fix), --target=chromium can be passed as an explicit argument in an npm script, eg. something like:

package.json:

{
   ...
   "scripts": {
     "web-ext-run-chrome": "web-ext run --target=chromium"
     ...
   },
   ...
}

and all the rest of the cli options passed from the config file.

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

No branches or pull requests

4 participants