fix: introduce robust env configuration (#165)

The #81 pull request
introduced a nasty bug where we weren't using prod.

The TL;DR of the bug is that `sed` was replacing both the initial
assignment and the pattern, such that this code:

```javascript
    const placeholder = 'MLAB_PROJECT_PLACEHOLDER';
    return placeholder === 'MLAB_PROJECT_PLACEHOLDER' ? 'mlab-staging' : placeholder;
```

became, at runtime:

```javascript
    const placeholder = 'mlab-oti';
    return placeholder === 'mlab-oti' ? 'mlab-staging' : placeholder;
```

This was fixed in #164
but `sed` substitution is fragile. We need more robustness.

I spent time thinking about this and this is what I concluded:

1. The biggest issue of the `sed` substitution is not the substitution
per se, but a *lack of debuggability* by which the substitution only
happens when deploying and cannot be tested locally.

2. Any solution that only applies when merging a pull request would
be morally as bad as that one. It could fail without notice.

3. We now have `scripts/build.js` so we can take advantage of that.

4. Using `scripts/build.js` to select a committed `env.prod.js` _or_
`env.staging.js` is a *bad idea* because they might drift, thus
putting us *back again* into a scenario where we only know something
is wrong after merging a pull request.

Based on this reasoning, with this diff I am proposing what I think
is the most robust solution to the problem at hand:

1. We autogenerate `dist/js/env.js` from `scripts/build.js`

2. `scripts/build.js` *requires* a command line argument identifying
the target deployment environment and this value is *parsed* and we
reject invalid configurations (furthermore, it is always required to
specify the argument so prod and staging cannot drift)

3. `dist/js/env.js` *only* contains a *single* variable named
`mlabEnvName` and that can be *either* "prod" or "staging" (and it
must always be like that, so drift is *not possible*)

4. The codebase *continues* to select the correct configuration
inline depending on `mlabEnvName` values (which preserves the
principle of locality and ensures the code that selects the config
is *close to* the code it has effects upon)

5. Now the codebase *rejects* invalid or missing `mlabEnvName`
and the "not configured case" leads to *hard failure*

6. Both deployment paths use the same pattern `npm run build -- $name`
where `$name` is either "prod" or "staging" (this is arguably
better than before where the staging deployment relied on a default)

As a bonus, deploy `console.log` aggressively. This follows the
*rule of transparency*. The operator must be able to easily and
clearly see what URLs we're using. Easy inspection solves many
problems and empowers to perform better manual testing. (Also, a
user can now more easily report bugs.)

Author: bassosimone

.github/workflows/firebase-hosting-merge.yml

@@ -23,9 +23,7 @@ jobs:
         with:
           node-version: '20'
       - run: npm ci
-      - run: npm run build
-      - name: Substitute M-Lab project placeholder
-        run: sed -i 's/MLAB_PROJECT_PLACEHOLDER/mlab-oti/g' dist/js/app.js
+      - run: npm run build -- prod
       - uses: FirebaseExtended/action-hosting-deploy@v0
         with:
           repoToken: '${{ secrets.GITHUB_TOKEN }}'

.github/workflows/firebase-hosting-pull-request.yml

@@ -22,7 +22,7 @@ jobs:
         with:
           node-version: '20'
       - run: npm ci
-      - run: npm run build
+      - run: npm run build -- staging
       - uses: FirebaseExtended/action-hosting-deploy@v0
         with:
           repoToken: '${{ secrets.GITHUB_TOKEN }}'

README.md

@@ -22,7 +22,7 @@ The `mlab-speedtest` repository implements the https://speed.measurementlab.net/
 * Install website build dependencies for your operating system and environment
   * Node >= v20.0 - Using NVM: `nvm install v20.0`
   * Install libraries - `npm install`
-  * Build dist - `npm run build`
+  * Build dist - `npm run build -- staging`
   * Serving locally - `python3 -m http.server -d dist`
   * Firebase tools - `npm install -g firebase-tools`
 
@@ -37,7 +37,7 @@ Translations for this site are managed in the [Open Technology Fund's Localizati
 2. If the filename uses a locale code (e.g., `de_DE`), add a mapping in `scripts/po-to-json.js` so it maps to the short code (e.g., `de_DE` -> `de`)
 3. Add the short language code to the `supported` array in `src/js/i18n.js`
    * If the language is RTL, also add it to the `rtlLanguages` array
-4. Rebuild: `npm run build`
+4. Rebuild: `npm run build -- staging`
 
 The build converts `.po` files to JSON in `dist/translations/`. At runtime, `i18n.js` selects the language automatically via the `?lang=` query parameter, `localStorage`, or browser preference.
 
@@ -47,7 +47,7 @@ To preview the site locally, we recommend using Python:
 
 ```
 npm install
-npm run build
+npm run build -- staging
 python3 -m http.server -d dist
 ```
 

scripts/build.js

@@ -49,7 +49,23 @@ function copyFile(src, dest) {
  * Main build process
  */
 function build() {
-  console.log('Building M-Lab Speed Test...\n');
+  // Read command line arguments
+  const argv = process.argv;
+  if (argv.length !== 3) {
+    console.log('usage: node scripts/build.js prod|staging');
+    process.exit(1);
+  }
+  const mlabEnvName = argv[2];
+  if (mlabEnvName !== 'prod' && mlabEnvName !== 'staging') {
+    console.log('usage: node scripts/build.js prod|staging');
+    process.exit(1);
+  }
+  console.log('Building M-Lab Speed Test...');
+  console.log('');
+  console.log(`DIST: ${DIST}`);
+  console.log(`Environment: ${mlabEnvName}`);
+  console.log(`SRC: ${SRC}`);
+  console.log('');
 
   // Clean dist directory
   if (fs.existsSync(DIST)) {
@@ -86,6 +102,11 @@ function build() {
   const msakPkg = path.join(NODE_MODULES, '@m-lab', 'msak', 'dist');
   copyFile(path.join(msakPkg, 'msak.min.js'), path.join(libDest, 'msak.min.js'));
 
+  // Write dist/js/env.js
+  const envJsPath = path.join(DIST, 'js', 'env.js');
+  console.log(`Writing ${envJsPath}...`)
+  fs.writeFileSync(envJsPath, `const mlabEnvName = "${mlabEnvName}";\n`);
+
   console.log('\nBuild complete! Output in dist/');
 }
 

src/index.html

@@ -200,6 +200,7 @@ <h3 data-i18n>Social</h3>
     <script src="libraries/ndt7.js"></script>
     <script src="libraries/msak.min.js"></script>
 
+    <script src="js/env.js"></script>
     <script src="js/i18n.js"></script>
     <script src="js/gauge.js"></script>
     <script src="js/app.js"></script>

src/js/app.js

@@ -129,12 +129,14 @@ const SpeedTest = {
   },
 
   /**
-   * Determine the M-Lab project based on a placeholder that is substituted
-   * during deployment.
+   * Determine the M-Lab project based on js/env.js, which we
+   * generate using `./scripts/build.js`.
    */
   mlabProject() {
-    const placeholder = 'MLAB_PROJECT_PLACEHOLDER';
-    return placeholder.includes('PLACEHOLDER') ? 'mlab-staging' : placeholder;
+    if (mlabEnvName !== 'prod' && mlabEnvName !== 'staging') {
+      throw Error('The mlabEnvName variable has not been configured!');
+    }
+    return mlabEnvName === 'prod' ? 'mlab-oti' : 'mlab-staging';
   },
 
   /**
@@ -149,22 +151,30 @@ const SpeedTest = {
 
   async runNdt7(sid) {
     const project = this.mlabProject();
+    console.log(`[ndt7] mlabProject: ${project}`);
+
     const tokenURL = `https://speed-backend.${project}.measurementlab.net/v0/token`;
+    console.log(`[ndt7] tokenURL: ${tokenURL}`);
+
     const locatePriorityURL = this.locatePriorityURLForProject(project);
+    console.log(`[ndt7] locatePriorityURL: ${locatePriorityURL}`);
 
     let token = null;
     try {
       const tokenResp = await fetch(tokenURL);
       const tokenData = await tokenResp.json();
       token = tokenData.token;
     } catch (err) {
-      console.warn('Failed to fetch token, running without priority access:', err);
+      console.warn('[ndt7] Failed to fetch token, running without priority access:', err);
     }
 
+    const loadbalancer = token ? locatePriorityURL : null;
+    console.log(`[ndt7] loadbalancer: ${loadbalancer}`);
+
     return ndt7.test(
       {
         clientRegistrationToken: token,
-        loadbalancer: token ? locatePriorityURL : null,
+        loadbalancer: loadbalancer,
         userAcceptedDataPolicy: true,
         uploadworkerfile: '/libraries/ndt7-upload-worker.js',
         downloadworkerfile: '/libraries/ndt7-download-worker.js',
@@ -176,7 +186,7 @@ const SpeedTest = {
       {
         serverChosen: (server) => {
           this.els.location.textContent = server.location.city + ', ' + server.location.country;
-          console.log('Testing to:', {
+          console.log('[ndt7] Testing to:', {
             machine: server.machine,
             locations: server.location,
           });
@@ -234,7 +244,7 @@ const SpeedTest = {
   async runMSAK(sid) {
     const client = new msak.Client('speed-measurementlab-net', '1.0.0', {
       onDownloadStart: (server) => {
-        console.log('Server: ' + server.machine);
+        console.log('[msak] Server: ' + server.machine);
         this.els.msakLocation.textContent = server.location.city + ', ' + server.location.country;
       },
       onDownloadResult: (result) => {