feat: add content script registration optional#2288
feat: add content script registration optional#2288eupthere wants to merge 6 commits intowxt-dev:mainfrom
optional#2288Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
pkg/wxt
|
@aklinker1 Right now
The issue mentions deduping as a nice to have. Since WXT already uses I can either:
Happy to do whichever you prefer. |
@wxt-dev/analytics
@wxt-dev/auto-icons
@wxt-dev/browser
@wxt-dev/i18n
@wxt-dev/is-background
@wxt-dev/module-react
@wxt-dev/module-solid
@wxt-dev/module-svelte
@wxt-dev/module-vue
@wxt-dev/runner
@wxt-dev/storage
@wxt-dev/unocss
@wxt-dev/webextension-polyfill
wxt
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
+ Coverage 79.74% 79.84% +0.09%
==========================================
Files 130 130
Lines 3802 3825 +23
Branches 860 867 +7
==========================================
+ Hits 3032 3054 +22
- Misses 686 687 +1
Partials 84 84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // @ts-expect-error: Allow using strings for permissions for MV2 support | ||
| manifest.optional_permissions.push(permission); | ||
| } | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
I’m not sure reduce makes sense in addOptionalPermission, since that helper is just ensuring the array exists, deduping, and mutating the manifest. Did you mean refactoring the optionalContentScripts.forEach(...) logic, rather than the manifest.optional_permissions.push(permission) part?
There was a problem hiding this comment.
Yeah i mean refactor optionalContentScripts.forEach(...) because you have nested forEach and it doesn't look good.
I think you can omit entire addOptionalPermission, but now i have better idea.
Let's use .reduce like it's in line 414 for hostPermissions, it'll be good enough for there.
I should add this comment above, but as i mentioned here, i want to do it in other way previously 😆
There was a problem hiding this comment.
Could you suggest the change please? I can't think of how using Array.prototype.reduce can help with readability. The optionalContentScript is pretty much identical to the existing code:
// Runtime content scripts
const runtimeContentScripts = contentScripts.filter(
(cs) => cs.options.registration === 'runtime',
);
runtimeContentScripts.forEach((script) => {
script.options.matches?.forEach((matchPattern) => {
addHostPermission(manifest, matchPattern);
});
});
There was a problem hiding this comment.
Yeah, I'm not sure what you're requesting here either @PatrykKuniczak
There was a problem hiding this comment.
Ok guys, my bad .reduce() here, isn't good.
But IMO a little better is:
contentScripts
.filter((cs) => cs.options.registration === 'optional')
.flatMap(script => script.options.matches || [])
.forEach((matchPattern) => {
addOptionalHostPermission(manifest, matchPattern);
});But if you think it isn't let's leave what it is.
| delete manifest.host_permissions; | ||
| } | ||
|
|
||
| function moveOptionalHostPermissionsToOptionalPermissions( |
There was a problem hiding this comment.
The same like for addOptionalPermission
| if (script.options.registration === 'optional') { | ||
| addOptionalHostPermission(manifest, matchPattern); | ||
| } else { | ||
| addHostPermission(manifest, matchPattern); | ||
| } |
There was a problem hiding this comment.
What if i want to use both in the same time?
There was a problem hiding this comment.
I think that’s a totally fair point, and I hadn’t considered that case.
export default defineContentScript({
matches: ['https://my-app.com/*'],
registration: 'optional',
// ...
});The original issue’s proposed API treats registration as a single option for the whole content script, so it doesn’t currently allow choosing required vs optional behavior per match.
If we want to support both without introducing breaking changes, maybe we could extend the API, for example by adding a new optional field to separate optional matches from required ones. That way the current API keeps working, while still leaving room for mixed-permission content scripts.
Maybe something like this..
export default defineContentScript({
matches: ['https://required.com/*'],
optionalMatches: ['https://optional.com/*'],
})There was a problem hiding this comment.
export default defineContentScript({ matches: ['https://required.com/*'], optionalMatches: ['https://optional.com/*'], })
Yeah i was thinking about that, because we can't close our users to THIS or THIS approach, especially if on native approach, you can have both.
There was a problem hiding this comment.
@marcellino-ornelas @aklinker1 Any feedback on this approach?
There was a problem hiding this comment.
Hmm... I like optionalMatches. As I was reviewing this, I was starting to get confused how we would describe the difference between runtime and optional, having a separate property might make more sense.
There was a problem hiding this comment.
I like the optionalMatches here as well 🙌🏽 This is actually something were supporting right now in our custom module wasnt sure how to express this at the time of my issue tho soo left this out. Being able to support both like this is amazing and solves all of our usecases ❤️
There was a problem hiding this comment.
although how would the registration option work here now? you need registration: 'runtime' soo it doesn't hit the manifest and optional content scripts need the registration: 'runtime' but your matches may want to use registration: 'manifest'
Unless were saying here all matches goes to manifest and all optionalMatches goes to runtime. The only weird case would be if you wanted your matches configured to runtime and have them go to host_permissions which I haven't found a use case for that yet but maybe someone out there does?
- Clarified error message for content scripts not registered at runtime.
- Clarify the error message for invalid `optional` content scripts without `matches`.
- Use proper type instead of const
|
|
||
| ## Optional Host Registration | ||
|
|
||
| When using `registration: 'optional'`, WXT adds the script's `matches` to | ||
| `optional_host_permissions` instead of `host_permissions`. You must request host | ||
| access before registering/executing the script at runtime. |
There was a problem hiding this comment.
This doesn't go here. Let's put it here instead: https://wxt.dev/guide/essentials/content-scripts.html
We should add an entire section that covers all three options.
I need to update the example from above to not use defineContentScript, you should use an unlisted script for that use-case.
| if (script.options.registration === 'optional') { | ||
| addOptionalHostPermission(manifest, matchPattern); | ||
| } else { | ||
| addHostPermission(manifest, matchPattern); | ||
| } |
There was a problem hiding this comment.
Hmm... I like optionalMatches. As I was reviewing this, I was starting to get confused how we would describe the difference between runtime and optional, having a separate property might make more sense.
| // @ts-expect-error: Allow using strings for permissions for MV2 support | ||
| manifest.optional_permissions.push(permission); | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah, I'm not sure what you're requesting here either @PatrykKuniczak
|
The only other improvement here I would recommend is to dedupe example optional_host_permissions: [
"https://*.internal-server.com"
]This way one host gives you access to anything internal. But in our content scripts were more specific like It would be sweet if:
No big deal if not we can easily change our custom module to remove the dups if not 😃 Although I think overall this behavior would be sweet for not only optional content scripts but also regular matches too |
| hostPermission: string, | ||
| ): void { | ||
| manifest.optional_host_permissions ??= []; | ||
| if (manifest.optional_host_permissions.includes(hostPermission)) return; |
There was a problem hiding this comment.
Left this comment separately but you might want to consider using MatchPatterns here soo not only do you check whether its included but you can also check to see if a more wildcard host captures this host more broadly
see this comment for more details:
#2288 (comment)
There was a problem hiding this comment.
Love the deduping idea, and thanks for the implementation tips!
I am considering using @webext-core/match-patterns as it's already in wxt core dependency. I just need some feedback on the pattern matching API : #2288 (comment)
4 participants
Overview
Adds support for
registration: "optional"for content scripts so host match patterns are treated as optional origins instead of required host permissions.Related Docs:
MDN optional_host_permissions
Chrome Extensions Docs MV2 - Optional Permissions
What changed:
"optional".registration: "optional"scripts fromcontent_scripts(same dynamic-registration model as runtime scripts).matchesintooptional_host_permissionsinstead ofhost_permissions.optional_host_permissionsare merged intooptional_permissionswhen targeting MV2."optional"follows runtime-style content script reload flow.matches"optional"registration mode and describe intended optional-host runtime usage.Please review this:
I intentionally kept
registration: 'runtime'as the only mode that can omit matches, to preserve manual injection flows (for examplebrowser.scripting.executeScript({ target: { tabId } ... })) where URL match patterns are not needed in entrypoint options.registration: "optional"requires matches because this mode derives origin scope from matches to populateoptional_host_permissions(and MV2 conversion tooptional_permissions).Could you confirm whether this assumption about the original runtime exception is correct, and whether keeping optional stricter is the intended design?
Manual Testing
wxt core test pass
bun run --filter wxt test runGenerated Manifest
cd packages/wxt-demo npm run build:chrome-mv2 npm run build:chrome-mv3MV2
{ "manifest_version": 2, "name": "wxt-demo", "version": "1.0.0", "icons": { "16": "icons/16.png", "32": "icons/32.png", "48": "icons/48.png", "128": "icons/128.png" }, "permissions": ["storage"], "default_locale": "en", "web_accessible_resources": [ "iframe-src.html", "unlisted.js", "content-scripts/ui.css" ], "background": { "scripts": ["background.js"] }, "browser_action": { "default_title": "Popup", "default_popup": "popup.html" }, "options_ui": { "open_in_tab": false, "page": "options.html" }, "sandbox": { "pages": ["example.html", "sandbox.html"] }, "content_scripts": [ { "matches": ["https://*.duckduckgo.com/*"], "css": ["content-scripts/automount.css"], "js": ["content-scripts/automount.js", "content-scripts/ui.js"] }, { "matches": ["<all_urls>"], "js": ["content-scripts/example-tsx.js"] }, { "matches": ["*://*.google.com/*"], "js": ["content-scripts/iframe.js"] }, { "matches": ["*://*.crunchyroll.com/*"], "js": ["content-scripts/location-change.js"] }, { "matches": ["*://*/*"], "js": ["content-scripts/main-world.js"], "world": "MAIN" } ], "optional_permissions": ["https://example.com/*"] }MV3
{ "manifest_version": 3, "name": "wxt-demo", "version": "1.0.0", "icons": { "16": "icons/16.png", "32": "icons/32.png", "48": "icons/48.png", "128": "icons/128.png" }, "permissions": ["storage", "sidePanel"], "default_locale": "en", "web_accessible_resources": [ { "resources": ["iframe-src.html", "unlisted.js"], "matches": ["*://*.google.com/*", "*://*.example.com/*"] }, { "resources": ["content-scripts/ui.css"], "use_dynamic_url": true, "matches": ["https://*.duckduckgo.com/*"] } ], "background": { "service_worker": "background.js" }, "action": { "default_title": "Popup", "default_popup": "popup.html" }, "options_ui": { "open_in_tab": false, "page": "options.html" }, "sandbox": { "pages": ["example.html", "sandbox.html"] }, "side_panel": { "default_path": "sidepanel.html" }, "content_scripts": [ { "matches": ["https://*.duckduckgo.com/*"], "css": ["content-scripts/automount.css"], "js": ["content-scripts/automount.js", "content-scripts/ui.js"] }, { "matches": ["<all_urls>"], "js": ["content-scripts/example-tsx.js"] }, { "matches": ["*://*.google.com/*"], "js": ["content-scripts/iframe.js"] }, { "matches": ["*://*.crunchyroll.com/*"], "js": ["content-scripts/location-change.js"] }, { "matches": ["*://*/*"], "js": ["content-scripts/main-world.js"], "world": "MAIN" } ], "optional_host_permissions": ["https://example.com/*"] }Related Issue
This PR closes #2239