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

feat: introduce a --firefox-preview flag for 'web-ext run' #2436

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

willdurand
Copy link
Member

@willdurand willdurand commented Jun 13, 2022

This flag allows to enable the Firefox Dev Preview for Manifest V3 (at the moment, given that was my main use case). I've been light on testing but it covers enough. I don't think we need to be super fancy right now (personally).

@willdurand willdurand requested a review from rpl June 13, 2022 13:17
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

Thanks Will, as I anticipated you I think this is a actually a great idea and the PR looks great as is.

We should create a followup issue in the mozilla/extension-workshop repo to add a section for this option in the web-ext command reference here https://extensionworkshop.com/documentation/develop/web-ext-command-reference/

And I think that as part of that it may be worth also to consider calling this option out as part of documentation page providing guidance for transitioning or developing mv3 extensions in Firefox.
wdyt?

In the meantime I'll file the issue and I'll mention in its description that we coordinate merging the changes to the web-ext references with the releasing on npm of a new web-ext version that provides this new cli option.

@Rob--W
Copy link
Member

Rob--W commented Jun 14, 2022

Let's call it --enable-mv3 or something from which it's obvious that mv3 is enabled with it?

Side note: Firefox Preview is the name of the initial test version of the GeckoView-based Firefox for Android.

@rpl
Copy link
Member

rpl commented Jun 14, 2022

Let's call it --enable-mv3 or something from which it's obvious that mv3 is enabled with it?

Side note: Firefox Preview is the name of the initial test version of the GeckoView-based Firefox for Android.

There were a couple of reasons why we ended up settling on --firefox-preview:

  • this is Firefox only, it doesn't have any effect on the Chromium extension runner
  • if we call it --something-mv3 (I think I have initially said to William that I was likely going to prefer something like --firefox-mv3-devprefix or something like that), then we would also become an obsolete cli option soon enough and we would need to remove it in a new major release, and so we were settling on --firefox-preview=mv3 (with mv3 as the current default) to add an option that may actually make sense to keep for longer (e.g. use it to also enable other "developer preview" features, like event pages in MV2 and some future one we may need to intrduce)

I'm not sure I'd be worried that the new cli option may sound like a temporary code name assigned to old GeckView-based Firefox for Android versions, but given that we explicitly used the term "Developer Preview" to refer to to the experimental mv3 support locked behind a pref maybe we could tweak it a little bit into: --firefox-devpreview

@willdurand how --firefox-devpreview would sound to you?

@willdurand
Copy link
Member Author

@willdurand how --firefox-devpreview would sound to you?

too long :) --dev-preview would work for me, though

@willdurand
Copy link
Member Author

And I think that as part of that it may be worth also to consider calling this option out as part of documentation page providing guidance for transitioning or developing mv3 extensions in Firefox.
wdyt?

Yeah, I agree

@rpl
Copy link
Member

rpl commented Jun 14, 2022

@willdurand how --firefox-devpreview would sound to you?

too long :) --dev-preview would work for me, though

@willdurand how about --firefox-devpreview as the option name but with also a shorter alias like --devpreview or --dev-preview? 😁

@Rob--W
Copy link
Member

Rob--W commented Jun 14, 2022

In Chrome, MV3 is not behind a flag, so something like --enable-mv3 doing nothing would not be an issue.

Since conciseness is apparently desired, we could even go with
--mv3

This also leaves the option open to offer more specific mv3 flags in the future, e.g. --mv3=force (which could treat mv2 as mv3, to quickly allow devs to check whether the extension could work if they only change mv2 to mv3).

@willdurand
Copy link
Member Author

@willdurand how about --firefox-devpreview as the option name but with also a shorter alias like --devpreview or --dev-preview? 😁

I don't think an alias would work because if we refer to the alias instead of the full flag name, let's just not use an alias.

I think this discussion means --firefox-preview is the best choice so far given we want to (1) have a Firefox specific flag, (2) not too long and (3) that we can reuse for something else in the future.

(FTR the initial flag name I had was --preview because of our "dev preview")

Since conciseness is apparently desired, we could even go with --mv3

This also leaves the option open to offer more specific mv3 flags in the future, e.g. --mv3=force (which could treat mv2 as mv3, to quickly allow devs to check whether the extension could work if they only change mv2 to mv3).

I would be OK with --mv3 (short, self-explanatory, little to no side effects on non-Firefox browsers) but that goes against (3) above.

@rpl
Copy link
Member

rpl commented Jun 16, 2022

@willdurand how about --firefox-devpreview as the option name but with also a shorter alias like --devpreview or --dev-preview? grin

I don't think an alias would work because if we refer to the alias instead of the full flag name, let's just not use an alias.

I think this discussion means --firefox-preview is the best choice so far given we want to (1) have a Firefox specific flag, (2) not too long and (3) that we can reuse for something else in the future.

(FTR the initial flag name I had was --preview because of our "dev preview")

Since conciseness is apparently desired, we could even go with --mv3
This also leaves the option open to offer more specific mv3 flags in the future, e.g. --mv3=force (which could treat mv2 as mv3, to quickly allow devs to check whether the extension could work if they only change mv2 to mv3).

I would be OK with --mv3 (short, self-explanatory, little to no side effects on non-Firefox browsers) but that goes against (3) above.

would --devpreview (and ["firefox-mv3"] as the current default value) be a choice that matches all 3 points and be short and self-explanatory enough from all our 3 perspective?

In my opinion, this is shorter in its most common use cases, and it would still make it pretty clear if a devpreview feature being requested applies only to a specific browser.

This would also allow to extend the CLI option to more quickly enable experimental features in Chrome based browsers as well (just as an example, if the promises API in chrome was locked behind a CLI flag in Chrome then we could support a 'chrome-promised-api' devpreview value that would pass the chrome flag automatically, as this PR does for the Firefox about:config prefs to enable manifest_version 3 support).

As a side note, web-ext run does actually accept more than one browser target at the time, e.g. the following command would run a manifest version 3 extension on both Chrome and Firefox:

$ web-ext run --devpreview -s /path/to/test-mv3-extension/ -t firefox-desktop -t chromium

and so being able to extend a --devpreview cli option to the non-Firefox extension runners seems something that could turn out to be useful, and so a cli option that we could keep for longer than just the Firefox MV3 DevPreview phase.

@willdurand @Rob--W how that sounds to you?

@willdurand
Copy link
Member Author

@willdurand @Rob--W how that sounds to you?

Can we just stick to what we have in the current patch?

@willdurand
Copy link
Member Author

(rebase)

@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #2436 (7ec5a97) into master (78039fd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2436   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          31       31           
  Lines        1549     1552    +3     
=======================================
+ Hits         1545     1548    +3     
  Misses          4        4           
Impacted Files Coverage Δ
src/program.js 98.52% <ø> (ø)
src/cmd/run.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78039fd...7ec5a97. Read the comment docs.

@willdurand
Copy link
Member Author

willdurand commented Jun 17, 2022

As per our Slack discussion, I am landing this patch as is now.

@willdurand willdurand merged commit 89ccc55 into master Jun 17, 2022
@willdurand willdurand deleted the preview-mv3 branch June 17, 2022 12:31
@willdurand
Copy link
Member Author

We should create a followup issue in the mozilla/extension-workshop repo to add a section for this option in the web-ext command reference here extensionworkshop.com/documentation/develop/web-ext-command-reference

I submitted a PR: mozilla/extension-workshop#1339

Rob--W added a commit to Rob--W/web-ext that referenced this pull request Aug 5, 2024
The customPrefs alias was introduced in mozilla#1039 as a direct alias,
but changed to a shallow copy in mozilla#2436 because the object was modified.
These changes have been dropped in mozilla#3136 but the swallow copy remained.

This patch completes the cleanup by reverting to a direct alias.
Rob--W added a commit that referenced this pull request Sep 20, 2024
The customPrefs alias was introduced in #1039 as a direct alias,
but changed to a shallow copy in #2436 because the object was modified.
These changes have been dropped in #3136 but the swallow copy remained.

This patch completes the cleanup by reverting to a direct alias.
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 this pull request may close these issues.

3 participants