-
Notifications
You must be signed in to change notification settings - Fork 396
Migrate to v3 #2833
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
base: main
Are you sure you want to change the base?
Migrate to v3 #2833
Conversation
9ec9eaf to
c315a89
Compare
Rob--W
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very early feedback based on the diff only. I did not check whether there may be other issues.
A significant difference between the pre-patch behavior and the patch is that the background page is now non-persistent; that means that the background page may suspend after a (short) period of inactivity. If the extension relies on global variables or a specific order of execution, or does "one-time" initialization when some of the background scripts execute, then the switch to MV3 may cause regressions. This is not uniquely MV2 vs MV3, in MV2 non-persistent background pages also exist (opt in via "persistent": false in the background object), but it is still a new change introduced in this PR.
It is also worth noting that host permissions were not automatically granted on install in MV3 until Firefox 128. This is significant for ESR115 users: Existing users will retain the permissions, but new users will not have any host permissions.
| @@ -0,0 +1,14 @@ | |||
| diff --git a/node_modules/webextensions-jsdom/src/nyc.js b/node_modules/webextensions-jsdom/src/nyc.js | |||
| index 4ce55b5..909b822 100644 | |||
| --- a/node_modules/webextensions-jsdom/src/nyc.js | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relation between this change and the purpose of the PR is unclear. Could you elaborate? In this case it looks like a test-only issue which is not that concerning, but generally extensions should use third-party libraries in unmodified form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to validate the manifest v3, I had to update all the dependencies. The latest webextension-jsdom package is unable to create the cache/testing folder and that introduces an intermittent failure in our tests.
As you see, I had to convert a mkdirp method from async to sync to be sure the folder exists before continuing with the rest of the tests.
|
Thank you Rob! I did a bit of testing and the code does have almost 0 states. The only one I spot so far is related to the isolation key and the 3rd commit is about that bit.
This is what I'm mostly concern about: the backward compatibility of this change. |
This patch is a starting point to migrate this add-on to manifest v3.