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

Optionally allow polyfilling the browser global even when used with a bundler #376

Closed
wants to merge 4 commits into from

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Mar 29, 2022

I think this file solves the issue in an extremely lightweight, non-breaking, trouble-free way:

  • the existing bundle is not affected
  • it's opt-in, easily
  • if a project imports webextension-polyfill/register and webextension-polyfill separately, it will not be bundled twice
  • it follows the conventional naming for imports that affect globals in Node: register
  • it matches exactly what users must currently do to set the browser global with a bundler

Benefits of having this file here

  • I don't have to create the same file in every project I use

  • it lets you safely polyfill the global without falling into this trap:

     import browser from "webextension-polyfill"
     window.browser = browser;
    
     import other from "module-that-expects-browser-global"

    One would think that by the 4th line, window.browser will be set, but in reality all dependencies are loaded/run before the current file is executed, so window.browser is undefined in "module-that-expects-browser-global". This is not common knowledge and it leads to frustration.

    This instead works flawlessly:

     import "webextension-polyfill/register"
     import other from "module-that-expects-browser-global"

Note about ESM (and lack thereof)

This file is meant to be used exclusively with bundlers, so require is fine here (and it actually matches the UMD it's already using)

Relation to #351

This offers an alternative way of using the polyfill:

  • explicitly via import "webextension-polyfill/register"
  • automatically via ProvidePlugin

Note that ProvidePlugin is Webpack-only so its support does not preclude this new usage, which is actually why I'm submitting this PR.

To do after approval

  • update the documentation

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #376 (587357c) into master (2ece178) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #376   +/-   ##
=======================================
  Coverage   91.03%   91.03%           
=======================================
  Files           1        1           
  Lines         145      145           
=======================================
  Hits          132      132           
  Misses         13       13           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ece178...587357c. Read the comment docs.

@fregante fregante changed the title WIP: Add polyfill entry point Optionally allow polyfilling the browser global even when used with a bundler Mar 29, 2022
@fregante fregante marked this pull request as ready for review March 29, 2022 04:19
@rpl
Copy link
Member

rpl commented Mar 31, 2022

This looks reasonable and it doesn't seem that it should have impacts on other ways to use the polyfill.

It would be nice to also cover it with a test case, I haven't yet looked into what kind of testing strategy we could use (e.g. if we can easily add a test case to one of the test suites we have or if we need a specific testing strategy) but in the meantime I wanted to ask you if you have any thoughts testing this change.

@fregante
Copy link
Contributor Author

The integration test code is a nit long so I'll need some time to look into it. I suppose the test would just assert whether globalThis.browser is a Proxy, but how to do that I don't know yet.

@oliverdunk
Copy link

I only just came across this but it is even more useful in Chrome's MV3, where none of the patterns currently suggested in the README are applicable when looking to polyfill globalThis:

We're currently working around this at 1Password with the following:

import browser from "webextension-polyfill";

if (!globalThis.browser) {
  globalThis.browser = browser;
}

That said, this MR looks like a perfect solution! Testing it was a little rough but I built the branch locally, tested it in our code and things seem to be working well 🙌

(P.S: If we end up merging this it'd be nice to explicitly call out MV3 in the README and point people towards it as the easiest path to getting things setup)

@fregante
Copy link
Contributor Author

Thanks for the note! I'll keep that in mind for #378 as well.

I haven't gotten around to adding tests for this and I'm not sure when I will have time to. I think this doesn't necessarily need it, because if this can't load the main polyfill then nothing else can.

@fregante
Copy link
Contributor Author

I won't be adding tests here, it's too much of an ask to build the whole testing harness to include webpack. Feel free to merge or close this

@fregante fregante closed this Feb 11, 2024
@fregante fregante deleted the patch-1 branch June 24, 2024 03:55
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.

Polyfills polyfill globals
3 participants