-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
feat(experimental): First-class support for excluding webextension-polyfill
#847
Conversation
✅ Deploy Preview for creative-fairy-df92c4 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
28fab4b
to
4c684e7
Compare
@@ -1,6 +1,6 @@ | |||
import { fakeBrowser } from '@webext-core/fake-browser'; | |||
import { describe, it, expect, beforeEach, vi, expectTypeOf } from 'vitest'; | |||
import { browser } from '../browser'; | |||
import { browser } from 'wxt/browser'; |
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.
Because we're using mkdist
now, we can import from wxt/browser
and it will be resolved from the NPM module instead of using the local file, and this will be replaced with wxt/browser/chrome
when using extensionApi: "chrome"
packages/wxt/src/browser/chrome.ts
Outdated
export const browser: AugmentedBrowser = | ||
// @ts-expect-error | ||
globalThis.browser?.runtime?.id == null | ||
? globalThis.chrome | ||
: // @ts-expect-error | ||
globalThis.browser; |
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.
Use browser
instead of chrome
if it exists. Both Firefox and Safari provide a browser
global, so browser
will be used by default for those two browsers.
For firefox MV2, browser
supports the promise API whereas chrome
doesn't (see comment), meaning this is necessary for Firefox.
For Safari, I'm not 100% sure if there's a difference, so I'll need feedback from people with safari extensions to see if it's the right move.
* effects the extension API included at RUNTIME - during development, types | ||
* depend on the import. | ||
* | ||
* NOTE: this only works if we import `wxt/browser` instead of using the relative path. |
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.
TODO: Add eslint rule to enforce this.
disableWebextensionPolyfill
with extensionApi
webextension-polyfill
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #847 +/- ##
==========================================
- Coverage 82.42% 82.30% -0.12%
==========================================
Files 112 113 +1
Lines 6280 6302 +22
Branches 1019 1028 +9
==========================================
+ Hits 5176 5187 +11
- Misses 1091 1101 +10
- Partials 13 14 +1 ☔ View full report in Codecov by Sentry. |
Tested new mode here: aklinker1/github-better-line-counts#34 Working like a charm! |
Warning
If you were previously using
disableWebextensionPolyfill
, here's how to migrate to the new setting:Apart of #784, this PR refactors the experimental flag for removing the
webextension-polyfill
from your bundled extension.Before, it was a blanket replacement - even if a NPM package depended on the polyfill, the polyfill would be stripped out of the bundle, often times breaking the package's functionality.
Now, WXT will use
wxt/browser/chrome
instead ofwxt/browser
. This new module just exports thechrome
orbrowser
global based on the runtime browser. If a NPM package imports the polyfill, the polyfill will be included and the NPM package will function normally!To enable this new functionality, add the following option to your
wxt.config.ts
:Note that this is an experimental feature, and I need help testing it out!