From fdc25c1f67d49eedcf48c9ba78d51cf131692139 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 19:25:50 -0700 Subject: [PATCH 01/19] chore: normalize filenames --- .github/{create-initial-issues.js => create_initial_issues.js} | 0 .../{automerge-prs-from-bot.yml => automerge_release_prs.yml} | 0 ...nal-commits-checker.yml => conventional_commits_checker.yml} | 0 .github/workflows/{node_ci.yml => tests.yml} | 0 bin/{change-ext-version.js => change_ext_version.js} | 0 package.json | 2 +- 6 files changed, 1 insertion(+), 1 deletion(-) rename .github/{create-initial-issues.js => create_initial_issues.js} (100%) rename .github/workflows/{automerge-prs-from-bot.yml => automerge_release_prs.yml} (100%) rename .github/workflows/{conventional-commits-checker.yml => conventional_commits_checker.yml} (100%) rename .github/workflows/{node_ci.yml => tests.yml} (100%) rename bin/{change-ext-version.js => change_ext_version.js} (100%) diff --git a/.github/create-initial-issues.js b/.github/create_initial_issues.js similarity index 100% rename from .github/create-initial-issues.js rename to .github/create_initial_issues.js diff --git a/.github/workflows/automerge-prs-from-bot.yml b/.github/workflows/automerge_release_prs.yml similarity index 100% rename from .github/workflows/automerge-prs-from-bot.yml rename to .github/workflows/automerge_release_prs.yml diff --git a/.github/workflows/conventional-commits-checker.yml b/.github/workflows/conventional_commits_checker.yml similarity index 100% rename from .github/workflows/conventional-commits-checker.yml rename to .github/workflows/conventional_commits_checker.yml diff --git a/.github/workflows/node_ci.yml b/.github/workflows/tests.yml similarity index 100% rename from .github/workflows/node_ci.yml rename to .github/workflows/tests.yml diff --git a/bin/change-ext-version.js b/bin/change_ext_version.js similarity index 100% rename from bin/change-ext-version.js rename to bin/change_ext_version.js diff --git a/package.json b/package.json index 341e75c..8b9c89e 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "release": "semantic-release", "build": "npm run web-ext -- build", "get-version": "echo $npm_package_version", - "change-ext-version": "bin/change-ext-version.js" + "change-ext-version": "bin/change_ext_version.js" }, "devDependencies": { "@commitlint/cli": "8.3.5", From 2ca33a3787043e5e3250573a0024cbd0ce00363b Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 19:53:57 -0700 Subject: [PATCH 02/19] refactor: rename icons/ to images/ --- src/browser_action/index.html | 2 +- src/browser_action/style.css | 2 +- src/{icons => images}/duck-pattern.jpg | Bin src/{icons => images}/duck-with-shades.png | Bin src/{icons => images}/duck-with-shades_130.png | Bin src/{icons => images}/duck-with-shades_16.png | Bin src/{icons => images}/duck-with-shades_32.png | Bin src/{icons => images}/duck-with-shades_64.png | Bin src/manifest.json | 12 ++++++------ 9 files changed, 8 insertions(+), 8 deletions(-) rename src/{icons => images}/duck-pattern.jpg (100%) rename src/{icons => images}/duck-with-shades.png (100%) rename src/{icons => images}/duck-with-shades_130.png (100%) rename src/{icons => images}/duck-with-shades_16.png (100%) rename src/{icons => images}/duck-with-shades_32.png (100%) rename src/{icons => images}/duck-with-shades_64.png (100%) diff --git a/src/browser_action/index.html b/src/browser_action/index.html index 22df316..64a5834 100644 --- a/src/browser_action/index.html +++ b/src/browser_action/index.html @@ -7,7 +7,7 @@
DDG Best Tracker Blocker extension logo

DDG Best Tracker Blocker

diff --git a/src/browser_action/style.css b/src/browser_action/style.css index a50fdc5..bc7342a 100644 --- a/src/browser_action/style.css +++ b/src/browser_action/style.css @@ -1,7 +1,7 @@ body { padding: 2rem; max-width: 25rem; - background: repeat url(../icons/duck-pattern.jpg); + background: repeat url(../images/duck-pattern.jpg); font-family: Arial, Helvetica, sans-serif; } diff --git a/src/icons/duck-pattern.jpg b/src/images/duck-pattern.jpg similarity index 100% rename from src/icons/duck-pattern.jpg rename to src/images/duck-pattern.jpg diff --git a/src/icons/duck-with-shades.png b/src/images/duck-with-shades.png similarity index 100% rename from src/icons/duck-with-shades.png rename to src/images/duck-with-shades.png diff --git a/src/icons/duck-with-shades_130.png b/src/images/duck-with-shades_130.png similarity index 100% rename from src/icons/duck-with-shades_130.png rename to src/images/duck-with-shades_130.png diff --git a/src/icons/duck-with-shades_16.png b/src/images/duck-with-shades_16.png similarity index 100% rename from src/icons/duck-with-shades_16.png rename to src/images/duck-with-shades_16.png diff --git a/src/icons/duck-with-shades_32.png b/src/images/duck-with-shades_32.png similarity index 100% rename from src/icons/duck-with-shades_32.png rename to src/images/duck-with-shades_32.png diff --git a/src/icons/duck-with-shades_64.png b/src/images/duck-with-shades_64.png similarity index 100% rename from src/icons/duck-with-shades_64.png rename to src/images/duck-with-shades_64.png diff --git a/src/manifest.json b/src/manifest.json index 4d81949..bf61fc0 100644 --- a/src/manifest.json +++ b/src/manifest.json @@ -4,9 +4,9 @@ "description": "Tracker Blocker extension, part of DDG's interview process", "version": "1.5.1", "icons": { - "16": "icons/duck-with-shades_16.png", - "32": "icons/duck-with-shades_32.png", - "64": "icons/duck-with-shades_64.png" + "16": "images/duck-with-shades_16.png", + "32": "images/duck-with-shades_32.png", + "64": "images/duck-with-shades_64.png" }, "permissions": ["webRequest", "webRequestBlocking", ""], "background": { @@ -22,9 +22,9 @@ ], "browser_action": { "default_icon": { - "16": "icons/duck-with-shades_16.png", - "32": "icons/duck-with-shades_32.png", - "64": "icons/duck-with-shades_64.png" + "16": "images/duck-with-shades_16.png", + "32": "images/duck-with-shades_32.png", + "64": "images/duck-with-shades_64.png" }, "default_popup": "browser_action/index.html", "default_title": "DDG Best Tracker Ext" From b123f70ddfb41dd0c5790ea38b4b73690409a9ad Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 19:55:58 -0700 Subject: [PATCH 03/19] chore: remove content scripts until needed --- src/content/index.js | 1 - src/manifest.json | 8 -------- 2 files changed, 9 deletions(-) delete mode 100644 src/content/index.js diff --git a/src/content/index.js b/src/content/index.js deleted file mode 100644 index 151c69d..0000000 --- a/src/content/index.js +++ /dev/null @@ -1 +0,0 @@ -console.info('This is comming from content script!'); diff --git a/src/manifest.json b/src/manifest.json index bf61fc0..f053757 100644 --- a/src/manifest.json +++ b/src/manifest.json @@ -12,14 +12,6 @@ "background": { "page": "background/index.html" }, - "content_scripts": [ - { - "matches": [""], - "all_frames": true, - "run_at": "document_start", - "js": ["content/index.js"] - } - ], "browser_action": { "default_icon": { "16": "images/duck-with-shades_16.png", From 7464fccaa831a51b3c0a8ee395dae6eac93e0909 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 21:50:19 -0700 Subject: [PATCH 04/19] fix: remove the large image from the final bundle --- package.json | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 8b9c89e..49f106e 100644 --- a/package.json +++ b/package.json @@ -145,7 +145,10 @@ }, "webExt": { "sourceDir": "src", - "artifactsDir": "dist" + "artifactsDir": "dist", + "ignoreFiles": [ + "images/duck-with-shades.png" + ] }, "dependencies": {} } From f8ed3a2e163acc07d9f82cb5f3a85495789eb66b Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 22:27:44 -0700 Subject: [PATCH 05/19] chore: clean up package.json file --- package.json | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 49f106e..dab4068 100644 --- a/package.json +++ b/package.json @@ -5,6 +5,9 @@ "description": "Second test project for DDG - Tracker blocker extension", "type": "module", "main": "", + "engines": { + "node": ">=13" + }, "repository": { "type": "git", "url": "git+https://github.com/lfilho/ddg-test-project.git" @@ -16,7 +19,7 @@ }, "homepage": "https://github.com/lfilho/ddg-test-project#readme", "scripts": { - "develop": "npm run web-ext -- run --bc --url $(pwd)/__tests__/shared/test_page.html", + "develop": "web-ext run --bc --url $(pwd)/__tests__/shared/test_page.html", "format": "prettier --write \"**/*.{js,json,css,md}\"", "//eslint is configured with prettier, so it will check and fix js formatting too": "", "lint": "eslint \"**/*.js\"", @@ -25,10 +28,9 @@ "start": "npm run develop", "test": "NODE_ENV=test node --experimental-vm-modules node_modules/jest/bin/jest.js", "test:watch": "npm test -- --watch", - "web-ext": "web-ext", "//Release related scripts:": "", "release": "semantic-release", - "build": "npm run web-ext -- build", + "build": "web-ext build", "get-version": "echo $npm_package_version", "change-ext-version": "bin/change_ext_version.js" }, @@ -78,7 +80,7 @@ "lint-staged": { "*.{json,css,md}": "npm run format --", "*.js": [ - "eslint --fix" + "npm run lint:fix" ] }, "prettier": { From 1d31f7e082dacece958d5ffff688d4f9532a0789 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Mon, 15 Jun 2020 22:28:53 -0700 Subject: [PATCH 06/19] docs: bring docs closer to a final state --- README.md | 34 ++++++++++++----- docs/INTERVIEW.md | 62 +++++++++++++++++++++++++++++++ docs/development/ARCHITECTURE.md | 42 ++++++++++++++++----- docs/development/README.md | 63 ++++++++++++++++++-------------- docs/development/RELEASES.md | 21 ++++++++--- 5 files changed, 170 insertions(+), 52 deletions(-) create mode 100644 docs/INTERVIEW.md diff --git a/README.md b/README.md index b47bd4a..f2edc31 100644 --- a/README.md +++ b/README.md @@ -1,24 +1,38 @@ -[![semantic-release](https://img.shields.io/badge/%20%20%F0%9F%93%A6%F0%9F%9A%80-semantic--release-e10079.svg)](https://github.com/semantic-release/semantic-release) ![Tests](https://github.com/lfilho/ddg-test-project/workflows/Tests/badge.svg) +[![semantic-release](https://img.shields.io/badge/%20%20%F0%9F%93%A6%F0%9F%9A%80-semantic--release-e10079.svg)](https://github.com/semantic-release/semantic-release) +[![Conventional Commits](https://img.shields.io/badge/Conventional%20Commits-1.0.0-yellow.svg)](https://conventionalcommits.org) # DDG Tracker Blocker -_Submission for DDG as part of hiring process_ +Simple browser extension to identify and block trackers. -Simple browser extension to identify and block trackers +## A submission for DuckDuckGo as part of their hiring process 🐣➡🊆 + +Interviewers and reviewers please see [INTERVIEW.md](/docs/INTERVIEW.md) for remarks and considerations with the interview process in mind. ## Quick installation and tests -//TODO +Clone or download this repo and then run `npm install` within it. You will node >= 13 installed. + +### See the extension in action + +Run `npm run develop`. This will open a Firefox instance up with the extension preloaded for you and our [test page](/__tests__/shared/test_page.html) loaded. That page will tell you if the extension is working or not. + +### Automated tests + +Run `npm test` for linting + unit test combo. -**Tests:** run `npm test` for linting + unit test combo +For more details on installing a development environment and technical decisions around it, check our [development docs](/docs/development/). -For more details on installing a development environment and technical decisions around it, check our [development docs](docs/development/). +## What are we solving? -## Problem and Solution +Please see the original [design doc](/docs/design/) for the problem and proposed solution that this project implements. -Please see the original [design doc](docs/design/) for the problem and proposed solution. +## Development details -## Developement details +Head over to [development docs](/docs/development/) folder for more details and rationale around: -Head over to [development docs](docs/development/) for more details on how to set up your dev environment as well some documented decisions around tooling and libraries of choice. +- **[docs/development/README.md](/docs/development/):** development environment, CI, tools and libraries being used for development. _Here be dragons._ +- **[docs/development/TESTING.md](/docs/development/TESTING.md):** testing strategies, constraints and recommendations. +- **[docs/development/RELEASES.md](/docs/development/RELEASES.md):** how and when we release a new version of the extension. +- **[docs/development/ARCHITECTURE.md](/docs/development/ARCHITECTURE.md):** an overview of our software architecture. diff --git a/docs/INTERVIEW.md b/docs/INTERVIEW.md new file mode 100644 index 0000000..81a85c7 --- /dev/null +++ b/docs/INTERVIEW.md @@ -0,0 +1,62 @@ +# Hello + +![](https://thumbs.gfycat.com/SorrowfulCooperativeKakarikis-size_restricted.gif) + +I hope so! + +Hello dear interviewers/reviewers. In this doc I describe a few decisions I took the liberty of making for the project only given the interview scenario. + +## Context and trade-offs + +I have never developed a browser extension before, so there was a lot of unknowns for me. In order to de-risk my deliverables, I prioritized getting simplistic working versions out, so I could frontload my learnings on all things WebExtensions, web-ext, and how to test it. That meant **deliberately** not investing time in some areas that in a real-world scenario I **would have**. For example: + +- **Firefox testing only.** Although Mozilla's best efforts with WebExtensions, it is not guaranteed it "just works" with the other browsers too, and I didn't want to enter the rabbit hole of testing each change in other browsers and debug problems in all of them. +- **Naive URL matching.** Right now our `List`s are simple `Set`s and the URL matching is done just by checking if a given URL is in the set. In the real world we should of course match against a pattern. +- **Hardcoded lists.** Right now we're using hardcoded values from `__url_fixtures.js` file (which is fine for testing), but ideally we would have a mechanism to (1) fetch a fresh deny list from the internet and bundle it with each release of our extension and/or (2) a mechanism to update the lists on-the-fly, while the extension is already running. Hardcoding it for now allowed me to move faster since I didn't have to (1) set up a local server to run my tests, (2) configure CSP and permissions on `manifest.json` files to allow me to do ajax calls. +- **No local server.** For testing that a `http(s)://` request is actually getting blocked, we ideally should have a local server running and make requests against that. Especially for testing the whitelisting logic (i.e.: requests that will go out to the network) we should be spamming the externail websites with our tests :-). But for this single-developer-and-only-a-few-requests-per-hour project I decided to just use ephemeral `https://itty.bitty.site` URLs to speed things up. +- **Sub-par security on Github Action's automerge** See #54 for more details. +- **ESM Modules.** See more detailed write up below. + +## Native ESM modules + +I'm using versions of both Firefox and Node to develop this project. That allows us to use ESM modules all the way from Node to the browser so I wanted to see how far I could get with that. +For real extension to be used by entire world, naturally, that wouldn't be possible. We would have to support older browsers. + +Fortunately, it should be trivial to move the project to something like [Snowpack](https://www.snowpack.dev/) or good ol' Webpack. + +Another reason we might not need a bundler: most projects need one to tree-shake, concatenate and minify files in order to speed up the webapp being downloaded to the browser. But in our case, it's an extension that will already be served zipped (63kb as of > v1.5.1) and installed in one go so the entire code would already be available in the user's filesytem/memory. No need to worry about network calls for this. + +## No transpilation + +Same reasoning from above applies here: not something needed only for this interview project, but surely we would like to transpile the code for production use. + +## Learnings / Blockers / Surprises + +After a load of reading, head scratching and debugging, here are a few learnings I had along the way: + +- While Firefox and most browsers support ESM modules, Firefox cannot load a `module.js` listed in `manifest.json`. For that you have to instead use a proxy html file with a `` in it. +- The blocking functionality of `webRequest` only works on http(s) protocols. I started my tests trying to block a local `file:///` and was scratching my head for too long until I learned that. That would demand a local server to be setup but instead I decided to use `https://itty.bitty.site`. +- `jsdom`, the default environment jest uses for tests, [don't support ESM modules yet](https://github.com/jsdom/jsdom/issues/2475). I lost a bit of time until I learned that and it prevented me to test, for example, the feedback form e2e. +- Although Firefox supports static class properties, eslint default parser doesn't yet. So I had to research a bit and learned that babel-eslint parser does, hence I switched eslint's parser to it at some point in the project. +- Web-ext uses an old version eslint which conflicts with the one in the actual project: https://github.com/mozilla/addons-linter/issues/3062#issuecomment-643825903. That meant I had to disable linting via web-ext tool. That meant our `manifest.json` no longer was going to get linted. I was ok with that decision since the manifest.json was pretty stable at that point and wasn't going to suffer more changes in the near term. +- Firefox was missing documentation in some things. I spent time scavenging the web and it seems it's an overall feeling across the developers out there. I created [a simple Pull Request](https://github.com/mozilla/extension-workshop/pull/649) to them in order to document one of those things I needed and only found much later on and a [feature request](https://github.com/mozilla/addons-linter/issues/3206). + +## What I would do differently next time + +Given the learnings above, even for a simple project like this, I would revisit some of my decisions and do some things differently: + +- I **would** use a transpiler. While Firefox has a great support for the modern features I wanted to use, I was naive in thinking that the testing libraries and tool I was using were also going to. + +## Comments about scope and estimation + +With the knowledge I gathered with this project, it's likely that my next similarly-scoped extension would take 1/3 or even 1/2 less time for me to develop. + +When I gave my estimations on the design doc, I knew the actual code to block a request would be quite simple (after glimpsing the `webRequest` API documentation) but I also knew that I would most likely face the headaches described above since it would be my first time dealing with browser extensions. + +My trade-off decisions above were also key in reducing the amount of moving parts and keep me focused on what mattered most. + +## Parting thoughts + +I had plenty of fun learning how to develop extensions and I'm happy to see Mozilla trying to standardize things with WebExtensions. + +If I were you I'd here me to keep the fun going 😇. diff --git a/docs/development/ARCHITECTURE.md b/docs/development/ARCHITECTURE.md index f81cab0..eccc4a0 100644 --- a/docs/development/ARCHITECTURE.md +++ b/docs/development/ARCHITECTURE.md @@ -1,21 +1,45 @@ # ARCHITECTURE -## WebExtension +## Code structure -// TODO +Our `src` is where the extension code lives and it follows the [Anatomy of a WebExtension](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Anatomy_of_a_WebExtension). +Familiarize yourself with the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions and https://extensionworkshop.com/ to build this WebExtension with us 🙂. + +Main files & folders: + +``` +__tests__/ → Tests written with jest framework +docs/ → You know it 😉 +src/ +├── background/ → Extension's long-running logic. Where the magic happens. ✹ +│   ├── index.html → Unfortunately needed to load index.js as a module +│   ├── index.js → Entrypoint. Loads request_blocker.js and start monitoring requests +│   ├── list_populator.js→ → Populates the list from our bundled values, fetches from remote sources, etc +│   ├── model/ → Our modeled entities +│   ├── request_blocker.js → Here's where we block those trackers! 💪 +│   ├── request_listener.js → Listens to the urls being requested and tell request_blocker.js whether to block the request or not +│   └── request_matcher.js → Checks urls against deny/allow list and tell request_listener.js if that url should be blocked or not +├── browser_action/ → The popup when we click the extension's icon in the toolbar. +│   ├── feedback_sender.js → Logic for sending the feedback +├── images/ → Icons and images to be used by browser and ourselves (e.g.: inside our popup page) +├── manifest.json → WebExtension's standard entrypoint for it all +└── shared/ → Modules and helpers to be shared across background, browser_action, etc. +``` + +Please follow that convention if we ever need, for example, [Content Scripts](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Anatomy_of_a_WebExtension#Content_scripts) or an [Options Page](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/user_interface/Options_pages). ## Error Handling -_See [issue 46](https://github.com/lfilho/ddg-test-project/issues/46) for more details on the reasoning for our Error handling._ +_See #46 for more details on the reasoning for our Error handling._ All errors in our codebase should extend `DDGError` class. This give us a few benefits: 1. **Error codes.** Each subclass' `this.name` will automatically serve as our unique error code. -1. **No duplicate errors.** That prevents things like `new Error('Invalid URL')` in `file1.js` and `new Error('This URL is invalid')` in `file2.js`. -1. **Better tests.** The above point make tests more robust by not having to rely on string matching for thrown errors. -1. **Better DX experience at scale.** The effort for creating a new Error type is minimal (a couple of lines). And after we have the error types we need, it's easier to reuse, debug, test and refactor since we don't depend on strings anymore. -1. **Human friendly.** This strategy forces us to think about `this.message` and make it developer/user friendly. It also allows us to easily translate our error messages if we want to. See the `DDGError` class for a simple example. -1. **Reusable error messages**. Since the messages are weel thought and translateable, we can easily surface them all the way to the UI if needed. That avoids having to have a separate map in the UI for "human friendly" messages. -1. **Better logging.** Since we have unique codes and a standard to follow now, we can make our Logger formate/print our errors to our liking. +2. **No duplicate errors.** That prevents things like `new Error('Invalid URL')` in `file1.js` and `new Error('This URL is invalid')` in `file2.js`. +3. **Better tests.** The above point make tests more robust by not having to rely on string matching for thrown errors. +4. **Better DX experience at scale.** The effort for creating a new Error type is minimal (a couple of lines). And after we have the error types we need, it's easier to reuse, debug, test and refactor since we don't depend on strings anymore. +5. **Human friendly.** This strategy forces us to think about `this.message` and make it developer/user friendly. It also allows us to easily translate our error messages if we want to. See the `DDGError` class for a simple example. +6. **Reusable error messages**. Since the messages are well thought and translatable, we can easily surface them all the way to the UI if needed. +7. **Better logging.** Since we have unique codes and a standard to follow now, we can make our Logger formate/print our errors to our liking. **Note**: To ensure we will continue to have those benefits, keep an eye out during PR reviewing time for colleagues mistakenly using `new Error()` or similar instead one of our errors. If we want to get fancy we could even consider writing an eslint plugin for that 🀓. diff --git a/docs/development/README.md b/docs/development/README.md index fc8a7b3..d71d215 100644 --- a/docs/development/README.md +++ b/docs/development/README.md @@ -1,6 +1,6 @@ # Development -_All your base are belong to us_ +_All your base are belong to us!_ Here are some decisions we have done for our development, feel free to suggest better ones. @@ -11,44 +11,53 @@ For details on our architecture, see [ARCHITECTURE.md](./ARCHITECTURE.md) instea ## Intallation -No mistery: `npm i`. +1. Install node >= 13 -- We're using native ESM modules. +2. Clone this repo and then run `npm install` within it. -## Tests +## Extension live reload and debugging -Tests are located under `tests/` folder. -The main entry point is `npm test`. That will run linting and then the actual test suit for you. -Check the npm scripts on `package.json` for what gets actually called behind that command. +Use `npm run develop` or `npm start` to run `web-ext run` behind the scenes. -## Linting and formatting +`web-ext` in an official tool from Mozilla that will + +- Spin up a browser instance (separate from your personal/already running one) +- Watch for file changes and automatically load your extension code in it + +Learn how you can debug the extension on the browser on [Mozilla's Extension Workshop website](https://extensionworkshop.com/documentation/develop/debugging/). -_See #35 for the initial work on this._ +## Tests -1. We use [lint-staged](https://npm.im/lint-staged) and [husky](https://npm.im/husky) to automatically run tests before commits. Check the `husky` and `lint-staged` sections in `packaging.json` for more details on what it does. -2. We use [prettier](https://npm.im/prettier) to auto style our code so you don't have to worry about it. ✹ -3. We use [eslint](https://npm.im/eslint) to lint (and fix, when possible) our code for common errors. It is configured to also report and fix `prettier` errors, making `eslint` our single entry point for static analysis. -4. We recommend also running auto-linting and formatting upon file save with your editor, so your staging and committing flow is faster and with less surprises. -5. Both `eslint` and `prettier` configurations are defined in `package.json`. +Here some basic commands to run tests. For more information and guidance on **testing**, see [TESTING.md](/docs/development/TESTING.md) -## ESM modules - No bundlers +- **Folder.** All tests are in the [`/__tests__/`](/__tests__/) folder. +- **Entrypoints.** + - **`npm run test`** or **`npm test`**. The main one. Will run `npm run lint` and then the actual test suit using `jest`. + - **`npm run test:watch`.** Will watch your files for changes and auto run `npm run test` when a file is changed. + - **`npm run lint`.** Will lint your javascript files. + - **`npm run lint:fix`.** Will lint your javascript files and auto-fix whatever it can. Automatically called before any commit. Details below. -For the purposes of this project/interview, I'll be using a modern version of Node and Firefox to develop and test the changes. -That allows us to use ESM modules all the way from Node to the browser. -If needed/time permitting, we can always add Snowpack or Webpack later have the usual transpilation done, but I wanted to save that time configuring all that boilerplate and slowing development down. +Check the npm scripts on [`package.json`](/package.json) for what gets actually called behind each command. -Another reason not to demand a bundler upfront is that since this project is an extension, not a webapp. So it's likely it won't get huge like most webapps nowadays, and will be download just once when installing the extension (and the occasional update). Hence we don't need to rush towards minification, bundling, tree-shaking, etc. +## Linting and formatting -## No babel +- We use **[lint-staged](https://npm.im/lint-staged)** and **[husky](https://npm.im/husky)** to automatically run tests before commits. Check their sections in [`package.json`](/package.json) for more details on what they do. +- We use **[prettier](https://npm.im/prettier)** to auto style our code so you don't have to worry about it. ✹ + - **`npm run format`.** Will auto format your js, json, css and markdown files using `prettier`. This command is called by lint-staged before any commit attempt. +- We use **[eslint](https://npm.im/eslint)** to lint (and fix, when possible) our code for common errors. It is configured to also report and fix `prettier` errors, making `eslint` our single entry point for static analysis. + - **`npm run lint:fix`.** Will lint your javascript files and auto-fix whatever it can. This command is called by lint-staged before any commit attempt. -In addition to the rationale above about ESM modules, I anticipate the javascript in this project will be mainly vanilla javascript, not complex enough to need much else transpilation -- the only real external dependency is the WebExtension API which is already implemented in most major browsers for more than 2 years (and it's something that Babel wouldn't be able to "transpile" anyway). +## Recommended dev setup -## CI +For your daily development, we recommend: -We're using Github Actions os our CI envirorment. Check the [.github folder](../../.github/workflows/) for what we are using. In sum: +- Having one terminal window running `npm run develop` +- Having one terminal window running `npm run test:watch` +- Running linting and formatting automatically upon file save with your editor/IDE, so your testing and committing flows are faster and with less surprises. -- `npm run test` on each PR. -- `npm run release` on each push to master, auto-generating a release, release notes, version bumps, etc. _See #36 for the initial work on it. And [the releases doc](./RELEASES.md) for more details on how releases are done._ +## CI -## Git flow +We're using Github Actions as our CI envirorment. Check the [workflows folder](/.github/workflows/) for what we are using. In sum, it: -We're using feature branches and PRs to merge them to master. -A branch can have as many commits as you like but PRs will be squashed into a single commit. So make your branches/PRs small and cohesive. This enables a more streamlined master's history, easier to rollback if needed, easier and faster to rebase, etc. +- Runs `npm run test` on each PR. +- Runs `npm run release` on each push to `master`, auto-generating a release, release notes, version bumps, etc. See the [RELEASES.md](/docs/development/RELEASES.md) for more details on how releases are done. +- Create a new PR with the updated files and automerge it diff --git a/docs/development/RELEASES.md b/docs/development/RELEASES.md index 844358b..dd4f2d8 100644 --- a/docs/development/RELEASES.md +++ b/docs/development/RELEASES.md @@ -1,13 +1,22 @@ # Releases -Upon any change landing on `master` branch, the following steps will be executed, culminating in a release for us. Everything is orchestrated by [this github action](../../.github/workflows/release.yml) and the `release` configuration section in `package.json`. +Upon any change landing on `master` branch, the following steps will be executed, culminating in a release for us. Everything is orchestrated by [the release workflow](/.github/workflows/release.yml) and the `release` configuration section in [`package.json`](/package.json). In a nutshell, here's what happens. 1. **`npm run release`**: That uses the semantic-release package to do the heavy lifting for us. 2. **`semantic-release`** will call a couple of plugins on its turn: - 2.1. **`commit-analyzer`**: will decide what makes a patch bump, a minor bump, or a major bump in our SemVer. It follows the `conventional commits` standard. - 2.2. **`release-notes-generator`**: does what it says. You can check any releases on Github to see the final output. - 2.3. **`exec`**: custom script executor. In our case, will call `web-ext` to build the extension and generate the .zip for us. - 2.4. **`npm`**: will update the package.json version (and publish it to npm registry, if we wanted). - 2.5. **`github`**: will tag and create a "release" according to github standards, including the .zip file above linked in it as a release asset. + 1. **`commit-analyzer`**: will decide what makes a patch bump, a minor bump, or a major bump in our SemVer. It follows the `conventional commits` standard. + 2. **`release-notes-generator`**: does what it says. You can check any releases on Github to see the final output. + 3. **`exec`**: custom script executor. In our case, will call `web-ext` to build the extension and generate the .zip for us. + 4. **`npm`**: will update the package.json version (and publish it to npm registry, if we wanted). + 5. **`github`**: will tag and create a "release" according to github standards, including the .zip file above linked in it as a release asset. + +## Semantic Releases and conventional commits + +We follow [SemVer](https://semver.org/). Automatically. 🀯. How? + +1. Upon any push to `master`, the `semantic-release` tool (run by the workflow above), will traverse all commits since last release and decide on a new semver version for our app. How? +2. By parsing each commit message against [conventional commits](https://conventionalcommits.org/) and decide what makes a patch, minor or major version bump. + +That means that we have to be diligent about our commit messages and follow the conventional commits standard above. From e5af50c633704099396857251b3b9136f82c4341 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 10:45:21 -0700 Subject: [PATCH 07/19] refactor: remove logging noise --- src/background/request_listener.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/background/request_listener.js b/src/background/request_listener.js index 5172db9..5bfc7e2 100644 --- a/src/background/request_listener.js +++ b/src/background/request_listener.js @@ -11,8 +11,7 @@ export default async function requestListener(requestDetails) { if (isUrlDenied) { const metric = new Metric(Metric.dimensions.REQUEST_BLOCKED, url); try { - const response = await MetricService.emit(metric); - Logger.debug(`Metric sent! Server returned status: ${response.status}`); + await MetricService.emit(metric); } catch (error) { Logger.error(error); } From 712b4fcf5e2df35dc82d8f056e7a21e7f7ef9d5f Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 10:50:20 -0700 Subject: [PATCH 08/19] chore: tidy package.json up --- package.json | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index dab4068..f616c75 100644 --- a/package.json +++ b/package.json @@ -19,21 +19,20 @@ }, "homepage": "https://github.com/lfilho/ddg-test-project#readme", "scripts": { - "develop": "web-ext run --bc --url $(pwd)/__tests__/shared/test_page.html", - "format": "prettier --write \"**/*.{js,json,css,md}\"", - "//eslint is configured with prettier, so it will check and fix js formatting too": "", + "start": "npm run develop", + "develop": "web-ext run --browser-console --url $(pwd)/__tests__/shared/test_page.html", + "build": "web-ext build", + "release": "semantic-release", + "format:fix": "prettier --write \"**/*.{js,json,css,md}\"", "lint": "eslint \"**/*.js\"", "lint:fix": "npm run lint -- --fix", "pretest": "npm run lint", - "start": "npm run develop", "test": "NODE_ENV=test node --experimental-vm-modules node_modules/jest/bin/jest.js", "test:watch": "npm test -- --watch", - "//Release related scripts:": "", - "release": "semantic-release", - "build": "web-ext build", "get-version": "echo $npm_package_version", "change-ext-version": "bin/change_ext_version.js" }, + "dependencies": {}, "devDependencies": { "@commitlint/cli": "8.3.5", "@commitlint/config-conventional": "8.3.4", @@ -78,9 +77,9 @@ } }, "lint-staged": { - "*.{json,css,md}": "npm run format --", + "*.{json,css,md}": "npm run format:fix --", "*.js": [ - "npm run lint:fix" + "npm run lint:fix --" ] }, "prettier": { @@ -151,6 +150,5 @@ "ignoreFiles": [ "images/duck-with-shades.png" ] - }, - "dependencies": {} + } } From 9917f1cdc3fb7083308a8f7fe7d8f9d4b26a8964 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 11:02:44 -0700 Subject: [PATCH 09/19] refactor: simplify List type logic with subclasses --- __tests__/src/background/model/list.spec.js | 6 ++---- src/background/model/allow_list.js | 4 +--- src/background/model/deny_list.js | 4 +--- src/background/model/list.js | 8 +------- src/shared/model/error.js | 4 ---- 5 files changed, 5 insertions(+), 21 deletions(-) diff --git a/__tests__/src/background/model/list.spec.js b/__tests__/src/background/model/list.spec.js index 34d4dfe..1609b94 100644 --- a/__tests__/src/background/model/list.spec.js +++ b/__tests__/src/background/model/list.spec.js @@ -1,16 +1,14 @@ import List from '../../../../src/background/model/list.js'; import { A_URL, - URL_IN_NO_LISTS, + URL_IN_NO_LISTS as ANOTHER_URL, SOME_URLS, } from '../../../../src/shared/__url_fixtures.js'; let list; -const ANOTHER_URL = URL_IN_NO_LISTS; beforeEach(() => { - const RANDOM_TYPE = List.types.DENY_LIST; - list = new List(RANDOM_TYPE); + list = new List(); }); describe('List', () => { diff --git a/src/background/model/allow_list.js b/src/background/model/allow_list.js index 25830b6..087aecb 100644 --- a/src/background/model/allow_list.js +++ b/src/background/model/allow_list.js @@ -1,7 +1,5 @@ import List from './list.js'; export default class AllowList extends List { - constructor() { - super(List.types.ALLOW_LIST); - } + type = List.types.ALLOW_LIST; } diff --git a/src/background/model/deny_list.js b/src/background/model/deny_list.js index 1b3864d..2f86240 100644 --- a/src/background/model/deny_list.js +++ b/src/background/model/deny_list.js @@ -1,7 +1,5 @@ import List from './list.js'; export default class DenyList extends List { - constructor() { - super(List.types.DENY_LIST); - } + type = List.types.DENY_LIST; } diff --git a/src/background/model/list.js b/src/background/model/list.js index 067a1c4..e96cbee 100644 --- a/src/background/model/list.js +++ b/src/background/model/list.js @@ -1,5 +1,3 @@ -import { ListTypeError } from '../../shared/model/error.js'; - export default class List { /* * Using a Set and mainly proxying the methods to their native ones. @@ -7,12 +5,8 @@ export default class List { * the API stays the same and the impact on the codebase is minimal. */ - constructor(type) { - if (!type) { - throw new ListTypeError(); - } + constructor() { this.list = new Set(); - this.type = type; } static get types() { diff --git a/src/shared/model/error.js b/src/shared/model/error.js index abf6257..79e2313 100644 --- a/src/shared/model/error.js +++ b/src/shared/model/error.js @@ -38,10 +38,6 @@ export class InvalidMetricDimensionError extends DDGError { message = t('Invalid metric dimension. See `Metric.dimensions` for values.'); } -export class ListTypeError extends DDGError { - message = t('List constructor needs a type. See `List.types` for values.'); -} - export class InputTooShortError extends DDGError { message = t('The value entered is too short.'); } From 7b16446ca05b2b9f0fcc44eb1cba05396ef95029 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 11:10:04 -0700 Subject: [PATCH 10/19] refactor: extract feedback selector to config file --- src/browser_action/feedback_sender.js | 13 +++++++------ src/browser_action/index.js | 7 +++++-- src/shared/config.js | 11 ++++++++++- src/shared/util/fake_fetch.js | 1 + 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/browser_action/feedback_sender.js b/src/browser_action/feedback_sender.js index 35b3db4..a51a97b 100644 --- a/src/browser_action/feedback_sender.js +++ b/src/browser_action/feedback_sender.js @@ -2,10 +2,11 @@ import { post } from '../shared/util/fake_fetch.js'; import { hasMinimalLength } from '../shared/util/validation.js'; import Logger from '../shared/util/logger.js'; import { InputTooShortError } from '../shared/model/error.js'; -import { FEEDBACK_ENDPOINT } from '../shared/config.js'; - -const COMMENT_FIELD_SELECTOR = '#comment-field'; -const MESSAGES_CONTAINER_SELECTOR = '#messages'; +import { + FEEDBACK_COMMENT_FIELD_SELECTOR, + FEEDBACK_MESSAGES_CONTAINER_SELECTOR, + FEEDBACK_ENDPOINT, +} from '../shared/config.js'; const $ = document.querySelector.bind(document); @@ -13,7 +14,7 @@ export async function submitFeedbackListener(event) { event.preventDefault(); try { - const feedback = $(COMMENT_FIELD_SELECTOR).value; + const feedback = $(FEEDBACK_COMMENT_FIELD_SELECTOR).value; validate(feedback); @@ -31,7 +32,7 @@ export async function submitFeedbackListener(event) { } function showMessage(text, className = '') { - const messagesContainer = $(MESSAGES_CONTAINER_SELECTOR); + const messagesContainer = $(FEEDBACK_MESSAGES_CONTAINER_SELECTOR); messagesContainer.className = className; messagesContainer.innerText = text; } diff --git a/src/browser_action/index.js b/src/browser_action/index.js index bde7bf8..057972e 100644 --- a/src/browser_action/index.js +++ b/src/browser_action/index.js @@ -1,5 +1,8 @@ import { submitFeedbackListener } from './feedback_sender.js'; +import { FEEDBACK_SUBMIT_BUTTON_SELECTOR } from '../shared/config.js'; -const SUBMIT_BUTTON_SELECTOR = '#submit'; const $ = document.querySelector.bind(document); -$(SUBMIT_BUTTON_SELECTOR).addEventListener('click', submitFeedbackListener); +$(FEEDBACK_SUBMIT_BUTTON_SELECTOR).addEventListener( + 'click', + submitFeedbackListener +); diff --git a/src/shared/config.js b/src/shared/config.js index 485a89b..e6ba4eb 100644 --- a/src/shared/config.js +++ b/src/shared/config.js @@ -1,4 +1,13 @@ const METRIC_ENDPOINT = 'https://fake-duckduckgo-endpoint.com/metrics'; const FEEDBACK_ENDPOINT = 'https://fake-duckduckgo-endpoint.com/feedback'; +const FEEDBACK_COMMENT_FIELD_SELECTOR = '#comment-field'; +const FEEDBACK_MESSAGES_CONTAINER_SELECTOR = '#messages'; +const FEEDBACK_SUBMIT_BUTTON_SELECTOR = '#submit'; -export { METRIC_ENDPOINT, FEEDBACK_ENDPOINT }; +export { + METRIC_ENDPOINT, + FEEDBACK_ENDPOINT, + FEEDBACK_COMMENT_FIELD_SELECTOR, + FEEDBACK_MESSAGES_CONTAINER_SELECTOR, + FEEDBACK_SUBMIT_BUTTON_SELECTOR, +}; diff --git a/src/shared/util/fake_fetch.js b/src/shared/util/fake_fetch.js index c8b8776..6b7fd2a 100644 --- a/src/shared/util/fake_fetch.js +++ b/src/shared/util/fake_fetch.js @@ -26,5 +26,6 @@ function fetch(url, payload) { const feedback = JSON.parse(payload.body).feedback; return feedback === 'fail' ? fail : success; } + return success; } From b0c906ba97459c28aaf9a32b30a825ace32db1d7 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 11:10:40 -0700 Subject: [PATCH 11/19] docs: minor comment rewording --- src/shared/metric_service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/metric_service.js b/src/shared/metric_service.js index be58504..cca7ff0 100644 --- a/src/shared/metric_service.js +++ b/src/shared/metric_service.js @@ -14,7 +14,7 @@ export default class MetricService { const metricString = `${metric.dimension}=${metric.value}`; // Here we would make a `fetch` request to our metric service - // For now, let's just log mimic an async request + // For now, let's just log and mock a fetch request Logger.debug(`Sending metric: ${metricString}`); return post(METRIC_ENDPOINT, metricPayload); } From c939252cb458a663a9bef2610fddbb3fde2c49f3 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 11:36:10 -0700 Subject: [PATCH 12/19] refactor: separate concerns inside Logger --- src/shared/util/logger.js | 43 +++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/src/shared/util/logger.js b/src/shared/util/logger.js index 03df3fc..02b5e5f 100644 --- a/src/shared/util/logger.js +++ b/src/shared/util/logger.js @@ -1,5 +1,3 @@ -const LOG_TOKEN_SEPARATOR = ' '; - export default class Logger { static get severities() { return Object.freeze({ @@ -29,24 +27,18 @@ export default class Logger { static log(message, severity = Logger.severities.INFO) { const timestamp = new Date().toISOString(); - const logFunction = getLogFunction(severity); - - const logPrefix = `[${severity.toUpperCase()}]${LOG_TOKEN_SEPARATOR}${timestamp}${LOG_TOKEN_SEPARATOR}`; - - let logMessage = `${logPrefix}${message}`; + const regularInfo = getFormattedLogLine(severity, timestamp, message); + const extraInfo = getAugumentedErrorMessage(message); + const logLine = [regularInfo, extraInfo].filter(Boolean).join('\n'); - // If an Error, augment it with extra info message, if available. - if (message instanceof Error) { - const extraInfo = message.extraInfo || ''; - logMessage = `${logMessage}\n[EXTRA INFO]: ${extraInfo}`; - } - logFunction(logMessage); + const logFunction = getLogFunction(severity); + logFunction(logLine); } } function getLogFunction(severity) { // If running node tests, don't log anything to avoid polluting the tests output: - // Optional chainig is not supported by eslint yet (would break our tests). Hence using good old && checks + // Optional chaining is not supported by eslint yet (would break our tests). Hence using good old && checks // @see https://github.com/eslint/eslint/pull/13416 let shouldLog = true; @@ -60,8 +52,29 @@ function getLogFunction(severity) { } if (!shouldLog) { - return () => {}; + const noop = () => {}; + return noop; } return console[severity].bind(console); } + +const TOKEN_SEPARATOR = ' '; +function getFormattedLogLine(severity, timestamp, message) { + const prefix = `[${severity.toUpperCase()}]`; + return [prefix, timestamp, message].join(TOKEN_SEPARATOR); +} + +function getAugumentedErrorMessage(message) { + let augumentWith = null; + + // If an Error, augment it with extra info message, if available. + if (message instanceof Error) { + const extraInfo = message.extraInfo; + if (extraInfo) { + augumentWith = `[EXTRA INFO]: ${extraInfo}`; + } + } + + return augumentWith; +} From 3577adfd9a428c24181c6af0e52fcfea8a085702 Mon Sep 17 00:00:00 2001 From: FutureDuck Date: Tue, 16 Jun 2020 11:52:04 -0700 Subject: [PATCH 13/19] refactor: tidy up feedback form markup --- src/browser_action/feedback_sender.js | 2 +- src/browser_action/index.html | 16 ++++++++++------ src/browser_action/style.css | 21 +++++++++++---------- src/shared/config.js | 6 +++--- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/browser_action/feedback_sender.js b/src/browser_action/feedback_sender.js index a51a97b..5474fc6 100644 --- a/src/browser_action/feedback_sender.js +++ b/src/browser_action/feedback_sender.js @@ -46,7 +46,7 @@ function showErrorMessage(message) { } function showWaitingMessage() { - showMessage('Sending...'); + showMessage('⏳ Sending...'); } function validate(feedback) { diff --git a/src/browser_action/index.html b/src/browser_action/index.html index 64a5834..d44869d 100644 --- a/src/browser_action/index.html +++ b/src/browser_action/index.html @@ -5,26 +5,30 @@ -
+
DDG Best Tracker Blocker extension logo -

DDG Best Tracker Blocker

+

DDG Best Tracker Blocker

-