search

MitsuhaKitsune / vuex-webextensions

A Vuex plugin to share store through webextensions components
MIT License
83 stars 30 forks source link

Bug/update detect browser #52

Closed TCashion closed 3 years ago

TCashion commented 3 years ago

Our organization found a bug where if a website uses a CSS selector id=browser on any HTML elements, a global browser variable gets created. This interferes with the current implementation of the detectBrowser function when running on Chrome because the conditional logic sees typeof browser !== "undefined" as a truthy expression. This causes vuex-webextensions to incorrectly define the browser object as Firefox, and prevents the content and background scripts from syncing as expected.

This was first noted on the site resellerclub.com on Chrome. When on that site, you can open devtools and type browser into the console. You will see an HTMLCollection object defined under that global variable name.

This PR updates the conditional logic to check for the InstallTrigger object (docs ) in order to verify whether a browser is Firefox. The idea for this implementation came from this stack overflow post.

TCashion commented 3 years ago

@MitsuhaKitsune Please let me know your thoughts! 🙏🏼

MitsuhaKitsune commented 3 years ago

Hi @TCashion,

On first place a lot of thanks on bug catching and your time to investigate the alternative on browser detection and send the PR.

I test it and seems working without problems, so I gona squash your commits and release a new npm build to apply your fix.

If someone found a bug with this, revert to previous version and tell me on issue tracker, on my tests didn't found any problem and I doubt that broke anything.

I will take the opportunity to update the dependencies and make some changes before the update, the new version with this fix included will be available in a maximum of 2 hours and I will notify you here when it is ready.

Once again, thank you very much for the PR, and sorry for the error, I was unaware that the browser variable could be manipulated in some environments.

TCashion commented 3 years ago

Once again, thank you very much for the PR, and sorry for the error, I was unaware that the browser variable could be manipulated in some environments.

No need to apologize! Thank you for the quick review.

TCashion commented 3 years ago

@MitsuhaKitsune I just wanted to check in since I did not see the new npm version get published. No hurry! I was just wondering whether you ran into issues with the package upgrades.

MitsuhaKitsune commented 3 years ago

Sorry for the delay, I upload the version 1.3.1 with this fix included to npm.

Now im updating to give support to vuex v4 and v5, chrome manifest v3 too, this gona take me some time, but for now the fix are available on v1.3.1 ^^

TCashion commented 3 years ago

Thanks, I really appreciate the responsiveness 👍🏼