lfilho · lfilho · Jun 16, 2020 · Jun 16, 2020 · Jun 16, 2020 · Jun 16, 2020
diff --git a/.github/create-initial-issues.js → .github/create_initial_issues.js b/.github/create-initial-issues.js → .github/create_initial_issues.js
diff --git a/.github/workflows/automerge-prs-from-bot.yml → .github/workflows/automerge_release_prs.yml b/.github/workflows/automerge-prs-from-bot.yml → .github/workflows/automerge_release_prs.yml
diff --git a/...orkflows/conventional-commits-checker.yml → ...orkflows/conventional_commits_checker.yml b/...orkflows/conventional-commits-checker.yml → ...orkflows/conventional_commits_checker.yml
diff --git a/.github/workflows/node_ci.yml → .github/workflows/tests.yml b/.github/workflows/node_ci.yml → .github/workflows/tests.yml
diff --git 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/__tests__/shared/test_page.html b/__tests__/shared/test_page.html
@@ -1,45 +1,47 @@
 <!DOCTYPE html>
 <html>
-
-<head>
-  <meta charset="UTF-8" />
-  <meta name="viewport" content="width=device-width" />
-  <title>Test page</title>
-  <style type="text/css">
-    body {
-      padding: 2rem;
-    }
-
-    #bad-page-iframe,
-    #good-page-iframe {
-      display: block;
-      width: 100%;
-      height: 13rem;
-      border: 0.2rem solid transparent;
-    }
-
-    #bad-page-iframe {
-      border-color: red;
-    }
-
-    #good-page-iframe {
-      border-color: green;
-    }
-  </style>
-</head>
-
-<body>
-  <h1 data-testid="test-page-header">🦆 Test page</h1>
-
-  <h2>Bad page iframe (should be blank):</h2>
-  <iframe id="bad-page-iframe"
-    src="https://itty.bitty.site/#bad_page/data:text/html;charset=utf-8;bxze64,XQAAAAI1AgAAAAAAAAAeGInmWR9D5qtrM4PFJv4W1okR98bzFbE2QlIWuIKxrmOKcpOUCz+bgN13tm1YwI65ckLaaMk97I0eK4ZJmz5rEyi5AYIrCihWPyWsU8imdwCjsAu9mVY4Uz3NDJSwS2tBy4fpBWIzlJM+PEIF1Dz7uHdB5dd0JkIYoHoNI51/xPcOHjaLtkdq929hapYOYARvUkxhKCAYZqoeWl/azB+/iRrItZhXoN2Xhb7cEouQ4ySG85i7cH0nZAJxZezxYYPzjYOKj1QECrYU2ufsndUGql9zsUXyBzVv3UCYzraVIpXLzEWabESPgYK1HY3biPhAiXzCU1i3msUdqKm0jl/HwqnVgJjhKaCsQvuv8pyy+VVkaz/XPVibLpmrbELCVUdG/jgKW2ig1pqjIAIoFb7YROo5+5026HlBBKlBJSJtJPiB5wAAbZMivZGwt72qmdA5R4jjQVr+djo2"
-    frameborder="0"></iframe>
-
-  <h2>Good page iframe (should not be blank):</h2>
-  <iframe id="good-page-iframe"
-    src="https://itty.bitty.site/#good_page/data:text/html;charset=utf-8;bxze64,XQAAAALrAQAAAAAAAAAeGInmWR9D5qtrM4PFJv4W1okR98bzFbE2QlIiIqEKRNhWozLGaVxy2UBVi6vb5PjLiS+KmhnoBI2zbVEi38FFqGt0V2dZ/n48NtEOjSTkOFXBuNLAKC6rlcwmvnHnUnMAWA2l/QImsEbNvvf1bv40vbtBzNp9F3TGp/HpcdlmwUSp59tjbjdUlRkOVnMxBaTPI6tnqjg9UBREwBH6Y4c6xLg53hJodPJyK8AysLdOEqC5OPFdGrHq7n6ViwKq90juHDM+UhFD8ug4iSu0Yo74yBMAo7Rtj+Jd5h9AkkjDCs/m4RMIP7KQKT4AldOuVvxaNDd4LcfbH/7lFxzMpv2FPyYxeR5ZmDMwE6422v7jh2OnV4nTcu42kWkhVBP7U06PxG1bBjmd5+5p11z/jK399tAktg=="
-    frameborder="0"></iframe>
-</body>
-
+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1" />
+    <title>Test page</title>
+    <style type="text/css">
+      body {
+        padding: 2rem;
+      }
+
+      #bad-page-iframe,
+      #good-page-iframe {
+        display: block;
+        width: 100%;
+        height: 13rem;
+        border: 0.2rem solid transparent;
+      }
+
+      #bad-page-iframe {
+        border-color: red;
+      }
+
+      #good-page-iframe {
+        border-color: green;
+      }
+    </style>
+  </head>
+
+  <body>
+    <h1 data-testid="test-page-header">🦆 Test page</h1>
+
+    <h2>Bad page iframe (should be blank):</h2>
+    <iframe
+      id="bad-page-iframe"
+      src="https://itty.bitty.site/#bad_page/data:text/html;charset=utf-8;bxze64,XQAAAAI1AgAAAAAAAAAeGInmWR9D5qtrM4PFJv4W1okR98bzFbE2QlIWuIKxrmOKcpOUCz+bgN13tm1YwI65ckLaaMk97I0eK4ZJmz5rEyi5AYIrCihWPyWsU8imdwCjsAu9mVY4Uz3NDJSwS2tBy4fpBWIzlJM+PEIF1Dz7uHdB5dd0JkIYoHoNI51/xPcOHjaLtkdq929hapYOYARvUkxhKCAYZqoeWl/azB+/iRrItZhXoN2Xhb7cEouQ4ySG85i7cH0nZAJxZezxYYPzjYOKj1QECrYU2ufsndUGql9zsUXyBzVv3UCYzraVIpXLzEWabESPgYK1HY3biPhAiXzCU1i3msUdqKm0jl/HwqnVgJjhKaCsQvuv8pyy+VVkaz/XPVibLpmrbELCVUdG/jgKW2ig1pqjIAIoFb7YROo5+5026HlBBKlBJSJtJPiB5wAAbZMivZGwt72qmdA5R4jjQVr+djo2"
+      frameborder="0"
+    ></iframe>
+
+    <h2>Good page iframe (should not be blank):</h2>
+    <iframe
+      id="good-page-iframe"
+      src="https://itty.bitty.site/#good_page/data:text/html;charset=utf-8;bxze64,XQAAAALrAQAAAAAAAAAeGInmWR9D5qtrM4PFJv4W1okR98bzFbE2QlIiIqEKRNhWozLGaVxy2UBVi6vb5PjLiS+KmhnoBI2zbVEi38FFqGt0V2dZ/n48NtEOjSTkOFXBuNLAKC6rlcwmvnHnUnMAWA2l/QImsEbNvvf1bv40vbtBzNp9F3TGp/HpcdlmwUSp59tjbjdUlRkOVnMxBaTPI6tnqjg9UBREwBH6Y4c6xLg53hJodPJyK8AysLdOEqC5OPFdGrHq7n6ViwKq90juHDM+UhFD8ug4iSu0Yo74yBMAo7Rtj+Jd5h9AkkjDCs/m4RMIP7KQKT4AldOuVvxaNDd4LcfbH/7lFxzMpv2FPyYxeR5ZmDMwE6422v7jh2OnV4nTcu42kWkhVBP7U06PxG1bBjmd5+5p11z/jK399tAktg=="
+      frameborder="0"
+    ></iframe>
+  </body>
 </html>
diff --git a/__tests__/src/background/__request_fixtures.js b/__tests__/src/background/__request_fixtures.js
@@ -8,14 +8,8 @@ const RANDOM_REQUEST_DETAILS = { url: A_URL };
 const ALLOWED_REQUEST_DETAILS = { url: GOOD_URL };
 const DENIED_REQUEST_DETAILS = { url: BAD_URL };
 
-const BLOCKING_RESPONSE = {
-  CANCELLED: { cancel: true },
-  NOT_CANCELLED: { cancel: false },
-};
-
 export {
   RANDOM_REQUEST_DETAILS,
   ALLOWED_REQUEST_DETAILS,
   DENIED_REQUEST_DETAILS,
-  BLOCKING_RESPONSE,
 };
diff --git 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', () => {
@@ -36,8 +34,8 @@ describe('List', () => {
 
   it('should tell if an url is on the list', () => {
     list.add(A_URL);
-    expect(list.has(A_URL)).toBe(true);
-    expect(list.has(ANOTHER_URL)).toBe(false);
+    expect(list.matches(A_URL)).toBe(true);
+    expect(list.matches(ANOTHER_URL)).toBe(false);
   });
 
   it('should clear the list', () => {

diff --git a/__tests__/src/background/request_listener.spec.js b/__tests__/src/background/request_listener.spec.js
@@ -8,9 +8,10 @@ import {
   RANDOM_REQUEST_DETAILS,
   ALLOWED_REQUEST_DETAILS,
   DENIED_REQUEST_DETAILS,
-  BLOCKING_RESPONSE,
 } from './__request_fixtures.js';
 
+import BlockingResponse from '../../../src/background/model/blocking_response.js';
+
 jest.mock('../../../src/background/request_matcher.js');
 jest.mock('../../../src/shared/metric_service.js');
 
@@ -23,13 +24,13 @@ describe('Request Listener', () => {
   it('should return a BlockingResponse with cancel: true for a tracker url', async () => {
     RequestMatcher.isDenied = jest.fn().mockReturnValue(true);
     const result = await requestListener(RANDOM_REQUEST_DETAILS);
-    expect(result).toEqual(BLOCKING_RESPONSE.CANCELLED);
+    expect(result).toEqual(BlockingResponse.CANCELLED);
   });
 
   it('should return a BlockingResponse with cancel: false for a non-tracker url', async () => {
     RequestMatcher.isDenied = jest.fn().mockReturnValue(false);
     const result = await requestListener(RANDOM_REQUEST_DETAILS);
-    expect(result).toEqual(BLOCKING_RESPONSE.NOT_CANCELLED);
+    expect(result).toEqual(BlockingResponse.NOT_CANCELLED);
   });
 
   it('should emit a metric when a request is blocked', async () => {

diff --git a/bin/change-ext-version.js → bin/change_ext_version.js b/bin/change-ext-version.js → bin/change_ext_version.js
diff --git a/docs/INTERVIEW.md 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 `<script type="module" src="module.js"></script>` 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 😇.