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

Migrate to webextension-polyfill (#11) #12

Merged
merged 5 commits into from
Aug 30, 2022
Merged

Migrate to webextension-polyfill (#11) #12

merged 5 commits into from
Aug 30, 2022

Conversation

vantezzen
Copy link
Contributor

@vantezzen vantezzen commented Aug 23, 2022

This PR migrates the package to the webextension-polyfill package to make it compatible with Firefox.

The package is used to not produce additional overhead by needing to use if (isChrome) ... else ... for every API call and instead using the unified, promise-based API provided by the polyfill.

The storage package API remains unchanged, so packages using this project don't need to update their code to support Firefox. A small info about adding an addon ID for Firefox was added to the README - if merged, this should be added to the docs.

To make the with-storage example work for Firefox I created a separate PR at PlasmoHQ/examples#16.

Fixes #11

@louisgv
Copy link
Contributor

louisgv commented Aug 29, 2022

Great work overall - since the polyfilled API are all async promise, perhap we can refactor some of the method to be plain async instead of a promise instead. Will take a look deeper in a bit!

@louisgv
Copy link
Contributor

louisgv commented Aug 29, 2022

Another issue I have with the polyfill is how much weight would it add to the overall bundle, and is there a way to strip out stuffs that's not storage related? :d....

Size ref:
https://bundlephobia.com/package/[email protected] - 2.8kB
https://bundlephobia.com/package/@plasmohq/[email protected] - 1.8kB

I will do another analysis post-bundle with esbuild. Perhaps the bundle mozilla is publishing is unminified.

Actually, disregard this. It won't inflate the bundle as it's installed as dependencies.

src/index.ts Outdated
export type StorageWatchEventListener = Parameters<
typeof chrome.storage.onChanged.addListener
typeof browser.storage.onChanged.addListener
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the const string typing

@louisgv
Copy link
Contributor

louisgv commented Aug 30, 2022

The discussion in here really makes me not want to adopt the polyfill: mozilla/webextension-polyfill#329

The polyfill is also preventing the unit test suite to run. And based on the thread above, it also introduces unwanted bug as well :...

On the other hand, without the polyfill, we will have to manually handle the callback xD............................

ARGH

@louisgv
Copy link
Contributor

louisgv commented Aug 30, 2022

Another problem encountered: the type of the polyfill somehow fuck all the jest mock typing up lol

@louisgv louisgv merged commit c9a0d4b into PlasmoHQ:main Aug 30, 2022
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.

[BUG] Never resolves on Firefox
2 participants