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

Add example using MV3 userScripts API #576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Dec 18, 2024

Description

Add example using MV3 userScripts API. And many other aspects of MV3 development, useful for #496.

Motivation

This example is part of the documentation of the userScripts API.

Additional details

Related issues and pull requests

@Rob--W Rob--W requested a review from dotproto December 18, 2024 14:53
"browser_specific_settings": {
"gecko": {
"id": "[email protected]",
"strict_min_version": "134.0a1"
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I put strict_min_version 134 because this is the first version of Firefox where the API landed, behind the extensions.userScripts.mv3.enabled preference. I may update it to 135 or 136 once we ship it by default on release.

@erosman
Copy link

erosman commented Dec 18, 2024

Note: "host_permissions" should include "file:///*", as many users (myself included) run personal userScripts on local files.

@Rob--W
Copy link
Member Author

Rob--W commented Dec 18, 2024

Note: "host_permissions" should include "file:///*", as many users (myself included) run personal userScripts on local files.

This is a demo extension, not a full-fledged user script manager. For it to be a complete user script manager, it would have to define the full GM.* / GM_* APIs, and also have a better UI. That goes beyond demonstrating the extension APIs and is left as an exercise to the reader.

@erosman
Copy link

erosman commented Dec 18, 2024

I understand.. the intension was to demonstrate that the MV3 userScripts API supports "file:///*" as well.

Copy link
Contributor

@dotproto dotproto left a comment

Choose a reason for hiding this comment

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

In this review I mostly identified small tweaks. The core functionality looks quite solid. Nice work, @Rob--W!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Rob--W, if you want to publish this example before the 135, I think we should add some installation instructions to the README that mention that you need to enable extensions.userScripts.mv3.enabled in about:config.

userScripts-mv3/README.md Outdated Show resolved Hide resolved
userScripts-mv3/README.md Outdated Show resolved Hide resolved
userScripts-mv3/README.md Outdated Show resolved Hide resolved
userScripts-mv3/README.md Outdated Show resolved Hide resolved

## What it does

This extension is an example of a [user script manager](https://en.wikipedia.org/wiki/Userscript_manager)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: end the sentence with a period.

Suggested change
This extension is an example of a [user script manager](https://en.wikipedia.org/wiki/Userscript_manager)
This extension is an example of a [user script manager](https://en.wikipedia.org/wiki/Userscript_manager).

userScripts-mv3/background.js Outdated Show resolved Hide resolved
Comment on lines 144 to 145
let worldIds = scriptsToRemove.map(s => s.id);
await browser.userScripts.unregister({ worldIds });
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 145 currently throws the following error:

Uncaught (in promise) Error: Type error for parameter filter (Unexpected property "worldIds") for userScripts.unregister.

Channel: Nightly
Version: 135.0a1 (2024-12-30) (aarch64)
OS: macOS 15.1 (24B83)

I looked through the current version of toolkit/components/extensions/schemas/user_scripts.json and the current version of UserScriptFilter only has a ids property.

It looks like this may be the result of some refactoring that occured during development. The script appears to work as expected if we change the body of the if clause as follows:

Suggested change
let worldIds = scriptsToRemove.map(s => s.id);
await browser.userScripts.unregister({ worldIds });
await browser.userScripts.unregister({ ids: scriptsToRemove });

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this! I've applied the suggestion and renamed scriptsToRemove to scriptIdsToRemove.

}
}

button.onclick = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to double check that re-assigning an onevent handler like this would remove the previous event handler, but it checks out! (mdn, whatwg spec)

examples.json Outdated Show resolved Hide resolved
userScripts-mv3/README.md Outdated Show resolved Hide resolved
The extension is an example of a
[user script manager](https://en.wikipedia.org/wiki/Userscript_manager).

This covers the following aspects to extension development:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
This covers the following aspects to extension development:
This example illustrates these aspects of extension development:

Copy link
Member Author

Choose a reason for hiding this comment

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

I accepted Simeon's suggestion below instead, which adds "demo" to get to This demo covers the following aspects to extension development:

Is this wording OK or is it preferable to use "illustrate"?

userScripts-mv3/README.md Outdated Show resolved Hide resolved

- Showing onboarding UI after installation.

- Designing background scripts that can restart repeatedly with minimal
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Rob--W as we seem to make the assumption that this is going to be rendered as a markdown document (e.g. use of hyperlink in the introductory paragraph) is there any need to impose line breaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

To make it easier to read when the file is not rendered.

userScripts-mv3/background.js Outdated Show resolved Hide resolved
userScripts-mv3/background.js Outdated Show resolved Hide resolved
userScripts-mv3/background.js Outdated Show resolved Hide resolved
userScripts-mv3/background.js Outdated Show resolved Hide resolved
userScripts-mv3/background.js Outdated Show resolved Hide resolved
userScripts-mv3/README.md Outdated Show resolved Hide resolved
userScripts-mv3/options.mjs Outdated Show resolved Hide resolved
Copy link
Member Author

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews and (comment) suggestions (@dotproto , @rebloor and @erosman). I have just applied them, with the changes visible at https://github.com/Rob--W/webextensions-examples/commits/82331d201638e099c597b04dd2d166f156713c97

I'm going to squash all commits into one to make it ready to merge.

The extension is an example of a
[user script manager](https://en.wikipedia.org/wiki/Userscript_manager).

This covers the following aspects to extension development:
Copy link
Member Author

Choose a reason for hiding this comment

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

I accepted Simeon's suggestion below instead, which adds "demo" to get to This demo covers the following aspects to extension development:

Is this wording OK or is it preferable to use "illustrate"?


- Showing onboarding UI after installation.

- Designing background scripts that can restart repeatedly with minimal
Copy link
Member Author

Choose a reason for hiding this comment

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

To make it easier to read when the file is not rendered.

Comment on lines 51 to 52
// Pretend that userScripts was not available, so that if the permission is
// restored, that permissions.onAdded will re-initialize.
Copy link
Member Author

Choose a reason for hiding this comment

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

Rephrased to:

    // Pretend that userScripts is not available, to enable permissions.onAdded
    // to re-initialize when the permission is restored.

Comment on lines 144 to 145
let worldIds = scriptsToRemove.map(s => s.id);
await browser.userScripts.unregister({ worldIds });
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this! I've applied the suggestion and renamed scriptsToRemove to scriptIdsToRemove.

@Rob--W Rob--W force-pushed the userScripts-mv3-sample branch from 82331d2 to 5aefc8a Compare January 15, 2025 22:40
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.

4 participants