search

dmlls / yang

Yang! - Yet Another Bangs anywhere Firefox extension
https://addons.mozilla.org/addon/yang-addon/
GNU General Public License v3.0
19 stars 3 forks source link

Add partial support for Chromium browsers #3

Open NyanKiyoshi opened 1 year ago

NyanKiyoshi commented 1 year ago

This adds partial support for chromium-based browsers (including Google Chrome). window.browser is only defined in firefox but window.chrome is available both in firefox and chromium and works just the same.

This only partially supports chromium as loadReplace is not supported in chromium:

I am not aware of workarounds for loadReplace, thus to make it work on chromium-based browsers it requires to remove the line.

Tested on:

dmlls commented 1 year ago

Hi @NyanKiyoshi, thanks a lot for the PR!

Even though Firefox does support chrome, it is not a one-to-one browser replacement.

So how do we feel about following MDN recommendations (see link above) and using WebExtension browser API Polyfill?

I know at this point this is a bit like killing a fly with a sledgehammer, but the extension will likely grow overtime (e.g., custom bangs are next), and using browser.* will also give us support for Safari.

About loadReplace, I guess we can keep this option only active for Firefox with a simple if check. I think it's nice to be able to press the back button to go back to the page you were on before executing the bang.

dmlls commented 1 year ago

@NyanKiyoshi since you opened the PR, would you like to add the polyfill?

It should be straightforward following the MDN docs. Since releases are not that frequent, I suggest we simply include the browser-polyfill.js file in the project instead of using npm.

NyanKiyoshi commented 1 year ago

Hi @dmlls, thank you for the details. I wasn't aware about those differences between the two implementations; it is indeed concerning as, just like you mentioned, it may negatively impact what your project can do in the future.

I will try to find some time in the coming days or weeks to introduce the polyfill library. I will also look deeper into the loadReplace issue.

I'm also aware that Google Chrome is throwing warnings as the project is using manifest v2 instead of v3 since Google Chrome deprecated the old manifest. It seems like it would be possible to easily upgrade but I would think it should be tackled as a separate PR and thus could be a technical debt/feature request to upgrade it. I do notice that Brave doesn't put such warnings, perhaps not all Chromium vendors are deprecating it.

dmlls commented 1 year ago

@NyanKiyoshi so actually, after reading https://github.com/mozilla/webextension-polyfill/issues/329 and the other issues that reference it, apparently the polyfill won't be of help anymore with MV3.

I wonder if then we should just focus on migrating to MV3 directly to avoid working twice in the future... Might be especially a good idea in the current state of the project, given that there are less than 100 lines of code to migrate.

I can open a PR in the following days and then it would be great if you could test it on Chrome. Does that sound good to you?

NyanKiyoshi commented 1 year ago

@dmlls sounds great 👍