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

--start-url option does not accept array anymore #1707

Closed
rensbaardman opened this issue Sep 17, 2019 · 6 comments · Fixed by #1801
Closed

--start-url option does not accept array anymore #1707

rensbaardman opened this issue Sep 17, 2019 · 6 comments · Fixed by #1801

Comments

@rensbaardman
Copy link

rensbaardman commented Sep 17, 2019

Is this a feature request or a bug?

bug

What is the current behavior?

When running

web-ext run --start-url example.com foobar.com

it does not run with the two urls as start urls, but says

Run the extension

Options:
   [...]
  --start-url, -u, --url           Launch firefox at specified page      [array]
   [...]

This command does not take any arguments

Notice that the command options docs (still) specifies an array as argument to --start-url, suggesting this should be possible. (see this line in the src)

What is the expected or desired behavior?

Prior to v3.1.0 (I think), this worked as expected: executing the command would run the extension in Firefox, starting with the two (or more) urls as open tabs.

The problem is probably the same regression as mentioned in #1652. There was also a comment explicitly mentioning this case. Note that the [array] specification in the cli options, contradicts an earlier comment that "The old behavior was not documented".

I suggest a similar fix as in that case, since disabling array support (requiring --start-url example.com --start-url foobar.com) would technically be a breaking change.

Else, the cli options man page should be changed (fine with me too if the solution would be too cumbersome to implement).

Version information (for bug reports)

  • Firefox version: 69.0
  • Your OS and version: macOS 10.11.6
  • Paste the output of these commands:
node --version && npm --version && web-ext --version

v10.16.0
6.11.3
3.1.1

@rensbaardman rensbaardman changed the title --start-url cli option does not accept array anymore --start-url option does not accept array anymore Sep 17, 2019
@Rob--W
Copy link
Member

Rob--W commented Sep 26, 2019

--start-url / -u is documented to take only one parameter:
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/web-ext_command_reference#--start-url

This is expected behavior, but I'm keeping the issue open until we have improved the output of web-ext run --help.

@rensbaardman
Copy link
Author

rensbaardman commented Sep 29, 2019

I just came across #797, which makes a valid point: if you can only set multiple URLs by adding multiple flags, it is not possible to do so with an environment variable. So this requires the ability to pass an array.

I am willing to try come up with a PR (which depends on fixing this yargs issue first: yargs/yargs#821, and then fixing this one would be nice too: yargs/yargs#1098). I can then also update the MDN docs. I might need some guidance later on, since I haven't looked at the code yet.

Does that work for you?

@Rob--W
Copy link
Member

Rob--W commented Sep 30, 2019

It's not clear what you're asking - are you going to fix the output of --help, or are you going to re-introduce support for multiple URLs in --start-url?

I have a very small preference for supporting exactly one value per --start-url / -u parameter, like this:

web-ext run -u example.com -u example.com/page2

.. but if yargs is fixed and we can fully rely on yargs for consistent behavior without too many hacks, then I wouldn't be against the following (previous behavior):

web-ext run -u example.com example.com/page2

@rensbaardman
Copy link
Author

Sorry for the confusion. My proposal is to accept arrays, so the previous behaviour (your second example). This would allow setting multiple URLs via environment variables.

I will try to tackle the yargs issues first. Depending on whether that is successful / easy, I will come back here with either a solution that accepts arrays, or just improving the --help output. Is that okay?

@Rob--W
Copy link
Member

Rob--W commented Sep 30, 2019

Either approach is fine to me.

@Rob--W
Copy link
Member

Rob--W commented Oct 24, 2019

We're going to publish a major version bump, because Node 8 is almost EOL, and we want to bump the version requirement to Node 10+. This issue is one of the issues that may result in backwards-incompatible behavior, so if you plan to submit a PR, please do so.

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

Successfully merging a pull request may close this issue.

2 participants