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. |
|
@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. |
| describe('optional_host_permissions', () => { | ||
| it('should keep optional_host_permissions as-is for MV3', async () => { | ||
| const expectedOptionalHostPermissions = ['https://google.com/*']; | ||
| const expectedOptionalPermissions = ['cookies' as const]; |
There was a problem hiding this comment.
this shouldn't be:
| const expectedOptionalPermissions = ['cookies' as const]; | |
| const expectedOptionalPermissions = ['cookies'] as const; |
??
'cookies' is literal anyway
or this isn't necessary at all
There was a problem hiding this comment.
Thanks for catching that. That was a type suppression from LLM, and I should’ve caught it before opening the PR.
I’ve replaced it with the proper type here: 8115eac
@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.68% -0.07%
==========================================
Files 130 130
Lines 3802 3825 +23
Branches 860 867 +7
==========================================
+ Hits 3032 3048 +16
- Misses 686 691 +5
- Partials 84 86 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const entrypoint = fakeContentScriptEntrypoint({ | ||
| options: { | ||
| registration: 'optional', | ||
| // @ts-expect-error |
There was a problem hiding this comment.
| // @ts-expect-error | |
| // @ts-expect-error Let's add a reason |
| // @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?
| 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 rather than change the existing meaning of registration: 'optional', for example by adding a new optional field to separate optional matches from required ones. That way the current API keeps working as proposed, while still leaving room for mixed-permission content scripts.
Maybe something like this..
export default defineContentScript({
matches: ['https://required.com/*'],
optionalMatches: ['https://optional.com/*'],
})- 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
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