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

Port to Vite and Typescript #103

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

Saghen
Copy link

@Saghen Saghen commented Jan 28, 2023

I recently discovered this fantastic project and, while getting the extension running on Firefox, I noticed the extension could use a fresh coat of paint. In general, I've attempted to improve the maintainability of the codebase through refactoring and the switch to Typescript while attempting to avoid complexity from a bundler.

  • Switch to Vite and Typescript with auto-reload development environment
  • Use ENVs for detection of testing and consent
  • Support both Manifest V2 and V3 for Firefox and Chrome respectively
  • Use package.json for all scripts
  • Remove dependency on ua-parser-js
  • Separate all calls to browser.storage.local into storage.ts
  • Misc refactoring of background, popup and consent

Currently, I'm using a fork of aw-client-js with support for fetch. Axios uses XHR behind the scenes which has been removed in Service Workers, a requirement for Manifest V3. Fetch is only supported on Node 18+ by default so I'll need to revisit those changes. https://github.com/Saghen/aw-client-js

@ErikBjare
Copy link
Member

ErikBjare commented Jan 28, 2023

Wow, awesome work. I'm a bit daunted by such a massive overhaul PR, but a few initial thoughts:

  • Keep the media submodule, it's there for a reason (to keep all assets in one place and avoid having img blobs in several repos).
  • Will the ua-parser-js deprecation/removal impact browser detection? (looks okay, but not sure)
  • Will Vite generate a non-minified, human-readable, output file? (Mozilla/Chrome addon review requires it)
  • What's the "{{chrome/firefox}}.manifest..." magic?
  • To what extend have you tested it? :)

This PR will probably supersede #81 #89 #100 (and more?)

@Saghen
Copy link
Author

Saghen commented Jan 28, 2023

Will the ua-parser-js deprecation/removal impact browser detection?

Tested on Firefox, Chrome and Opera but haven't tested on Safari

What's the "{{chrome/firefox}}.manifest..." magic?

Docs can be found here: https://vite-plugin-web-extension.aklinker1.io/guide/configuration.html#browser-specific-manifest-fields

To what extend have you tested it? :)

Tested the consent and popup on both Chrome and Firefox. Using it in a test environment alongside the production extension to see if there's any discrepancies

@Saghen
Copy link
Author

Saghen commented May 25, 2023

Heads up that I haven't abandoned this! It's been working great on my machine so far but would definitely like to get a few more eyes on it when it's closer to being ready. Waiting on the axios support for fetch axios/axios#5146 or will look to test and get my changes in https://github.com/Saghen/aw-client-js merged in before this PR can be merged.

@ErikBjare
Copy link
Member

No worries!

Just a heads-up though, there have been a few changes to master since you made this PR. You might want to carefully rebase, or resolve them some other way.

I would also appreciate it if you avoided any change to formatting rules. Big PRs like this are difficult to review as it is, so let's keep the formatting changes as minimal as possible so that the diff is readable.

Copy link
Member

@ErikBjare ErikBjare left a comment

Choose a reason for hiding this comment

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

Oof, apparently I had all these old review comments that I'd forgotten to submit. Sorry about the late response.

package.json Outdated Show resolved Hide resolved
.github/workflows/codeql.yml Show resolved Hide resolved
Makefile Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@ErikBjare
Copy link
Member

I merged the aw-client-js PR.

Curious if you'll continue to work on this, but no pressure or rush :)

@Saghen
Copy link
Author

Saghen commented Jan 31, 2024

Yep! Still planning to get this wrapped up

@Saghen
Copy link
Author

Saghen commented Mar 14, 2024

Surprised by some of my choices on the original PR, like removing the Makefile 😛 Rebased onto master but none of the changes, beside the README change seemed to be applicable. Other changes:

  • added back the Makefile
  • added precommit hook for formatting
  • fixed up the gitignore

I should have waited for your review before squashing the commits, sorry about that. Firefox supports Manifest v3 now so it may be worth upgrading to that in a subsequent PR, although there's some weird caveats wrt permissions that I might have to check on.

Makefile Outdated Show resolved Hide resolved
@Saghen Saghen requested a review from ErikBjare April 9, 2024 16:07
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
static/popup.html Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.prettierrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Saghen
Copy link
Author

Saghen commented May 20, 2024

Hey @ErikBjare would you be willing to take over this PR? Seems like some pretty minor things at this point and you should have edit access to the branch. I feel that would be quicker than the back and forth

@ErikBjare
Copy link
Member

ErikBjare commented May 20, 2024

@Saghen I haven't reviewed it properly because the diff is practically unreadable due to the formatting changes, making git not understand that files were moved and edited, not deleted and rewritten from scratch. It is possible one might be able to fix this by using rebase and adding a move commit before the other commits (as explained here), but I haven't done that before.

Given this, I don't think I could take it over in its current state without a lot of work. I would instead probably recreate the PR from scratch with inspiration taken from this, which I don't think would be faster, and which I would probably continue to procrastinate. Let's see if the above thing works first, I'll give it a shot.

Edit: Will take a little work, but this works:

#!/usr/bin/fish
git rebase -i $branch_head^  # edit first commit
git reset HEAD^

# For each moved file
set old static/popup.js && git restore $old
set new src/popup/main.ts && mv $new $new.new && git mv $old $new && mv $new.new $new

git commit --amend

@Saghen
Copy link
Author

Saghen commented May 20, 2024

Gotcha, let me know how that goes! I did rewrite everything from scratch except for popup.html and a couple others iirc so that's why git has not marked them as moved. And the formatting was to match the .editorconfig that was already in the repo but I'll remove that and take care of the review comments soon

.editorconfig Outdated Show resolved Hide resolved
@ErikBjare
Copy link
Member

Gotcha, let me know how that goes!

It worked, but was too cumbersome to complete.

I did rewrite everything from scratch except for popup.html and a couple others iirc so that's why git has not marked them as moved.

I realized that as I made my attempt and actually saw how significant the changes were. Even harder to review 😅, but I massively appreciate the effort of putting this together! 🥇

I'll give it another pass when the CI is passing and the formatting stuff is fixed. Might try to get another pair of eyes on this.

Copy link
Contributor

@BelKed BelKed left a comment

Choose a reason for hiding this comment

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

The only concern I have is that the Brave detection is not present in this version of the extension, so it would be a step back in the functionality of the extension...

Everything else looks fine to me 👍

@ErikBjare
Copy link
Member

ErikBjare commented May 23, 2024

@BelKed Yes, we'd have to check Brave detection and other recent commits to master (like 0b289c4) so that they're included correctly (as they may have disappeared in a rebase).

@Saghen
Copy link
Author

Saghen commented May 25, 2024

@ErikBjare comments resolved and rebased on master!

.github/workflows/build.yml Outdated Show resolved Hide resolved
@ErikBjare
Copy link
Member

Really excited about this, will try to find time to review and merge in the coming days.


// FIXME: Detect Vivaldi? It seems to be intentionally impossible
export const getBrowserName = () => {
if ((navigator as any).brave.isBrave()) {

Choose a reason for hiding this comment

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

TypeError: Cannot read properties of undefined (reading 'isBrave')

@powellnorma
Copy link

powellnorma commented Oct 3, 2024

I am unable to load the result of make build-firefox (firefox.zip) in firefox - It says This add-on could not be installed because it appears to be corrupt. . For chrome, loading chrome.zip works fine.

https://stackoverflow.com/q/39854127

In the browserconsole (ctrl+J) I see:

addons.xpi	WARN	Invalid XPI: Error: Cannot find id for addon

I was able to load the aw-watcher-web.zip from current master just fine.

Ok, it's probably due to missing:

  "browser_specific_settings": {
    "gecko": {
      "id": "{<uuid>}"
    }
  },

in manifest.json

Fixed in Saghen#2

mkdir -p artifacts && cd build && zip ../artifacts/chrome.zip -r *

zip-build-firefox:
mkdir -p artifacts && cd build && zip ../artifacts/firefox.zip -r *

Choose a reason for hiding this comment

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

Might make sense to add -FS to the zip command, so that not-anymore-existing files get removed in the .zip archive

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.

4 participants