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

Allow --start-url to accept a space separated list of URLs #797

Closed
kumar303 opened this issue Feb 9, 2017 · 18 comments
Closed

Allow --start-url to accept a space separated list of URLs #797

kumar303 opened this issue Feb 9, 2017 · 18 comments

Comments

@kumar303
Copy link
Contributor

kumar303 commented Feb 9, 2017

Is this a bug or feature request?

feature

What is the current behavior?

If you want to start Firefox with multiple tabs you can do so like this:

web-ext run --start-url mozilla.org --start-url addons.mozilla.org

but you can only put one URL in the environment variable:

WEB_EXT_START_URL=mozilla.org web-ext run

What is the expected or desired behavior?

You should be able to specify multiple URLs in the environment variable too, like this:

WEB_EXT_START_URL="mozilla.org addons.mozilla.org" web-ext run

I think the only change needed for this is to switch the --start-url type from string to array like the --ignore-files option.

Let's try to make it backwards compatible with multiple --start-url options if we can.

@wagnerand
Copy link
Member

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.

The mentor for this bug is: @kumar303

@gilbertginsberg
Copy link
Contributor

Hi @kumar303, Steven Gilbert here. Thank you for mentoring. Might I work on this issue? This would be my first patch.

@kumar303
Copy link
Contributor Author

Hi @gilbertginsberg, definitely! Here is some info about how to get started: https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md Let me know if you have questions.

@gilbertginsberg
Copy link
Contributor

gilbertginsberg commented Feb 27, 2017

@kumar303: I switched the --start-url type from string to array as you suggested, but that didn’t seem to bring about the desired behavior.

For context, I created a borderify.js WebExtension from Your first WebExtension to test on. And I can confirm that the following commands behaved correctly with both type: 'string' and type: 'array'. Meaning, Firefox opened successfully to the expected pages:

web-ext run --start-url mozilla.org --start-url addons.mozilla.org

WEB_EXT_START_URL=mozilla.org web-ext run

I also tried different urls, and they worked too.

But the WEB_EXT_START_URL="mozilla.org addons.mozilla.org" web-ext run command did not behave correctly, with either type: 'string' or type: 'array'. In both cases, Firefox opened like so:

screen shot 2017-02-26 at 9 56 33 pm

When I made the change from string to array I made sure to test the commands after the re-build completed.

I started going through the yarg docs for possible inspiration. Would I be going in the right direction there? Or is there another area of the codebase you might be able to point me to so I can investigate? Many thanks.

@kumar303
Copy link
Contributor Author

Yep, you're going in the right direction. Also take a look at how we define the --ignore-files option and also how it gets used.

@gilbertginsberg
Copy link
Contributor

gilbertginsberg commented Mar 1, 2017

Thanks @kumar303.

So in cmd/program.js I've defined the --start-url type from string to array.

And in cmd/run.js, I mirrored --ignore-files and defined startUrl with Array<string> here and here, and ?Array<string> here.

And in test.run.js I replaced 'www.example.com' with ['www.example.com'].

The tests passed, but I still didn't get the expected behavior of WEB_EXT_START_URL="mozilla.org addons.mozilla.org" web-ext run.

Also I've played with this code block. Would that be a worthwhile place to continue investigating?

Going forward I'll keep trying to better understand the codebase and yargs and trial-and-erroring. Hopefully with less errroring :P. And I need to spend some time getting better acquainted with Flow's syntax. I'll report back if any questions.

@kumar303
Copy link
Contributor Author

kumar303 commented Mar 1, 2017

The goal is definitely to get each URL sent as a separate arg to Firefox, like --url mozilla.org --url addons.mozilla.org. I'm not sure what will need adjusting to make that happen but I'd suggest using log.debug() statements (example) to figure out what's happening in your patch. If you run web-ext --verbose then you will see the debug statements.

@gilbertginsberg
Copy link
Contributor

Roger that @kumar303. I'll continue the investigation.

@gilbertginsberg
Copy link
Contributor

@kumar303: I've submitted PR #836 for this issue. Thank you for bringing to my attention log.debug and web-ext --verbose. They were a big help in getting me unstuck.

@kumar303
Copy link
Contributor Author

kumar303 commented Mar 9, 2017

I think the only change needed for this is to switch the --start-url type from string to array like the --ignore-files option.

d'oh, this is not working. It looks like we're hitting a bug in yargs: yargs/yargs#821

@gilbertginsberg let's see if we can get a fix in yargs before doing a workaround.

Because of the bug, --ignore-files is also broken: #854

@gilbertginsberg
Copy link
Contributor

Ah, okay okay. Sounds good, @kumar303.

@caitmuenster
Copy link

Hey @gilbertginsberg! How's it going with this bug?

@gilbertginsberg
Copy link
Contributor

gilbertginsberg commented Jun 13, 2017

Hi @caitmuenster: I submitted a PR that served as a workaround, but that wasn't ideal. Instead it'd be best to fix the underlying bug in yargs yargs/yargs#821, which appears is not yet resolved. So I'm keeping an eye out on that fix before proceeding.

@caitmuenster
Copy link

Sounds good. Thanks for the update!

@muffinresearch
Copy link
Contributor

@kumar303 Reading the context and the upstream issue I don't think it will be possible to fix yargs to take an array via an env var since env vars can only be strings - most shells don't seem to support exporting arrays.

@wagnerand
Copy link
Member

@kumar303 Given @muffinresearch's comment Is this issue still blocked by upstream?

@kumar303
Copy link
Contributor Author

kumar303 commented Aug 8, 2017

@muffinresearch I think it's possible because you can define the env var as a string with a space in it like this:

WEB_EXT_START_URL="mozilla.org addons.mozilla.org" web-ext run

However, since this is blocked upstream it is no longer a good first bug. I removed the labels.

@rpl
Copy link
Member

rpl commented Jun 6, 2022

I just double-checked if this issue or part of it was still valid, given that start-url has been changed to type string in in #1221 and #1707 fixed a regression on passing multiple urls over cli options (e.g. web-ext run --start-url www.mozilla.org developer.mozilla.org -s path/to/extension).

It looks that the environment variable can't be used to pass an array of values, but a config file like the following works:

module.exports = {
  run: {
    startUrl: ["https://www.mozilla.org", "https://developer.mozilla.org"],
  },
}

A config file can be specified on the cli using the --config option, or through the WEB_EXT_CONFIG environment variable.

Given that:

  • passing multiple start-url values through the environment variables is a corner case
  • that the current behavior technically depends from the behaviors implemented upstream by yargs, and that we went through more than a few yargs major releases by this time
  • and that the is a reasonable workaround (the config file, which may also be passed through WEB_EXT_CONFIG environment variable)

I'm closing this as fixed, because the original requests is already covered (web-ext run --start-url url1 url2 -s path/to/extension) and only a corner case related to doing the same from the WEB_EXT_START_URL isn't covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants