Repository navigation
feat: resolve the actual browser for browser-specific code generation - #2687
devtechedge wants to merge 8 commits into
Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
|
@devtechedge Let's resolve conflicts, yeah again 😆 I'll visit this PR, this week |
|
Thanks, Patryk, for checking in on this. I merged the latest main into the branch and resolved the conflict in resolve-config.test.ts. The PR is mergeable again now, and I have run the full test suite plus the lint and type checks locally, all of which pass. The Validate and pkg.pr.new workflow runs are still waiting on first-time contributor approval, so they have not run yet. Happy to adjust anything once you have had a chance to look at the branch. |
@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 #2687 +/- ##
==========================================
+ Coverage 80.44% 80.66% +0.21%
==========================================
Files 139 140 +1
Lines 4162 4173 +11
Branches 961 967 +6
==========================================
+ Hits 3348 3366 +18
+ Misses 716 710 -6
+ Partials 98 97 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
ChatGPT review: Thanks for the work on this PR. I think the overall approach is correct and the separation between Before merging, I would suggest addressing a few additional points to make the implementation more robust and easier to maintain. 1. Add integration tests for Safari/iOS manifest generationThe current regression tests verify the mapping and some manifest-specific behavior, which is a good start. However, I think it would be valuable to test the complete generated manifest for the For example, verify that:
Ideally, this should include an integration-level test rather than only testing individual properties. 2. Expand test coverage for custom browser mappingsSince For example: targetBrowserMap: {
'my-firefox': 'firefox',
'my-safari': 'safari',
}The tests should verify that custom targets correctly inherit the behavior of their mapped browsers, including manifest generation and browser-specific compatibility rules. This is particularly important because custom mappings are one of the main benefits of introducing this abstraction. 3. Revisit the fallback behavior and TypeScript typingThe current fallback: return targetBrowserMap[browser] ?? (browser as ActualBrowser);potentially introduces a type-safety issue. An unknown browser name can be returned as I would suggest considering one of the following approaches:
This would make the new abstraction more predictable and prevent the type system from suggesting stronger guarantees than the implementation actually provides. 4. Verify backward compatibilitySince the PR intentionally preserves the existing meaning of In particular:
This is important because introducing OverallI believe the proposed architecture solves the underlying problem correctly. My suggestions are primarily about strengthening regression coverage, ensuring type safety, and validating the behavior beyond the specific iOS/Safari scenario. I would consider these improvements worthwhile before merging, especially the custom mapping tests and the fallback typing issue. My suggestion |
Returning the raw label forced a type assertion on unknown --browser values. Chromium is the code WXT already generated for labels it does not recognize, so fall back to it explicitly and keep ActualBrowser honest. Also check own properties, so prototype keys passed to --browser cannot resolve to a function.
Compare complete manifests instead of single keys: an ios build must match the safari build for both manifest versions, custom labels must match the browser they map to, and Chrome-only fields must still appear for a chromium build so the negative assertions are not vacuous. Adds an e2e case that builds both labels, plus a unit test keeping BROWSER and the browser booleans on the raw label.
Explains which labels WXT knows, the Chromium fallback for anything else, and that output paths and import.meta.env.BROWSER keep the label you built with.
|
Fair list, and the typing point was a real bug, so thanks for flagging it 🙂 Unknown labels now resolve to On custom mappings, a label mapped to firefox or safari must produce the same complete manifest as that browser, in unit tests and an e2e. An ios build must match a safari build exactly for both manifest versions, with Safari's web extension converter ships with Xcode and only runs on macOS, so the manifest equality test is what covers validity here.
On 2648, I do not see an overlap, since it detects the browser at runtime for analytics, where one chromium build runs in Chrome, Edge, Brave or Opera, while this change is build time only and adds no dependency. I documented the option in the target different browsers guide, and everything is pushed with the suite, lint and type checks passing locally. Say the word if you want tests for zip naming or the launching path as well. |
I'll be back, probably tomorrow and i'll try to give you more feedback :) |
Reopening #2622, which was closed by accident when my fork was deleted. Rebased onto current main.
Overview
Fixes #2588.
Right now every build-time browser check compares
wxt.config.browseragainst a literal like"firefox"or"safari". Aniosbuild therefore misses every Safari check and generates the wrong manifest, even though it runs in Safari.This PR implements the three changes proposed in the issue:
InlineConfig#browserkeeps working exactly as before: it still drives the output directory template,import.meta.env.BROWSER, theCHROME/FIREFOX/SAFARI/EDGE/OPERAenv constants, the zip artifact name, and browser launching. Nothing changes for existing labels.InlineConfig#targetBrowserMapmaps--browserstrings to the actual browser. It defaults to:--browservaluechrome,edge,brave,opera,chromiumchromiumfirefoxfirefoxsafari,iossafariLabels missing from the map fall back to
chromium, which is the code WXT generated for unrecognized labels before this option existed, so their builds are unchanged until the project maps them to another browser.ResolvedConfig#actualBrowser: "chromium" | "firefox" | "safari"is resolved through that map and is now used for the manifest generation checks:use_dynamic_url(introduced in fix: don't set use_dynamic_url on browsers that don't support it #2581), MV2/MV3 key stripping, background/background scripts,version_name,browser_style/chrome_style,options_uiin Safari, side panels, and the Firefox warnings. The default manifest version (MV2 for Firefox and Safari) is also resolved off the actual browser now, so aniosbuild gets the same manifest as asafaribuild.The per-entrypoint
include/excludelists and per-browser entrypoint options still key off the raw--browserlabel, since those are user-authored values keyed by label.Manual Testing
resolve-config.test.ts: every default mapping resolves correctly, a customtargetBrowserMapentry wins over the defaults and can override one, unmapped labels fall back tochromium, inherited object properties are not resolved,actualBrowserappears on the resolved config, and the output directory still uses the raw label while the default manifest version keys off the actual browser.iosbuild must generate exactly the same manifest as asafaribuild for both manifest versions, with the Chrome-onlyuse_dynamic_urlandoptions_ui.open_in_tabfields asserted absent, and achromiumbuild keeps both so the negative assertions are not vacuous. A custom label mapped to firefox or safari must match that browser's manifest.safariandiosbuilt through the real config pipeline, which also covers the default manifest version and the output directory naming, and a customtargetBrowserMapset inwxt.config.ts.import.meta.env.BROWSERand the browser booleans on the raw label.Map Custom Browserssection to the target different browsers guide for the new option.manifest.tsback towxt.config.browserfails the ios and custom-mapping parity tests, while the chromium control case still passes.bun run check:defaultis green inpackages/wxt(ESLint, Oxlint, Publint, TypeScript), and the vitest suite inpackages/wxtpasses (594 tests).Related Issue
This PR closes #2588. Related discussion: #2581 (comment) and #2301.