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

Improved code #81

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

Conversation

GaurangTandon
Copy link

@GaurangTandon GaurangTandon commented Feb 8, 2022

Hi! I'm interested in contributing to this project. It seems to have been quite some time since this web watcher got any commits :'(

I did the following simple changes:

  1. Major change: Setup ESLint: quite essential as more contributors get involved in the project. Most changes in JS files are automatic lint changes, with minor manual variable renaming, etc.
  2. Minor change: Update package versions: just bumped them up
  3. Minor change: ES modules + Webpack: it is better than traditional commonjs requires. To be fair, given a certain minimum supported Chrome version, we won't need any transpilation using webpack, as ES6 works out of the box in background pages. But I leave that for another day.

I have verified the web watcher works on both Chrome and Firefox (sends correct tab data to the server in testing mode). I am not sure how to test it further.


After this MR, I intend to look into the MV3 migration and some more extensive testing strategies (perhaps using playwright?).
Please let me know how the maintainers plan to move this project forward. Thanks!

Setup ESLint linting
Switch to ESM modules using webpack
Update dependencies
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 8, 2022

This pull request fixes 1 alert when merging 849e3e7 into dddfc78 - view on LGTM.com

fixed alerts:

  • 1 for Expression has no effect

@ErikBjare
Copy link
Member

ErikBjare commented Feb 8, 2022

It seems to have been quite some time since this web watcher got any commits :'(

This is a good thing! That means it works as intended, and that we can focus our efforts elsewhere :)

ES modules + Webpack

We're actively trying to avoid complex bundling schemes (as webpack tends to entail) since new releases need to be reviewed by Google/Mozilla, and they don't like it when you use webpack/lots of dependencies.

If you could get ES modules to work with browserify, that would be preferable.

I intend to look into the MV3 migration

That would be very nice! I don't think it should be any trouble, hopefully :)

Also, looks like package-lock.json it outdated (CI fails). Try updating it.

@GaurangTandon
Copy link
Author

Thanks for your prompt reply @ErikBjare . Regarding webpack being complex, unfortunately, I cannot find a good way to use ES modules with browserify. However, I wish to ask you if webpack vs browserify matters in this simple use case. Would the bundle size be any different when using either?

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 8, 2022

This pull request fixes 1 alert when merging d7b6406 into dddfc78 - view on LGTM.com

fixed alerts:

  • 1 for Expression has no effect

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.

3 participants