a few changes to make A/B testing easier by dannycoates · Pull Request #537 · mozilla/send

dannycoates · 2017-08-25T00:27:57Z

also,

fixes #532
fixes #509
fixes #476
fixes #441
fixes #376

pdehaan · 2017-08-25T02:36:16Z

app/routes/index.js

+app.route('/legal', require('../templates/legal'));
+app.route('/error', require('../templates/error'));
+app.route('/blank', require('../templates/blank'));
+app.route('*', require('../templates/notFound'));


I haven't tested this locally yet, but... don't we need the /__version__ route, as well as the two health check OPs routes?

These are routes relative to the frontend js app. Server-only routes are in server/routes/index.js

pdehaan · 2017-08-25T02:38:18Z

app/routes/index.js

@@ -0,0 +1,17 @@
+const choo = require('choo');
+const download = require('./download');


nit: Is it weird that /app/storage.js uses import * from blah syntax but these other files use require('baz')?

It is weird. In order to run this code on the server, for server rendering, without transpiling some of it has to stick with require style. That being the case we might want to just go with node style require everywhere.

pdehaan · 2017-08-25T02:42:27Z

app/templates/file.js

+  if (hours >= 1) {
+    return `${hours}h ${minutes % 60}m`;
+  } else if (hours === 0) {
+    return `${minutes}m ${seconds}s`;


Cant remember if we have a bug or TODO for making these localize-able...

do now #538

pdehaan · 2017-08-25T02:44:26Z

app/templates/file.js

+  )}</span>
+            <span class="popup-yes" onclick=${deleteFile}>${state.translate(
+    'deletePopupYes'
+  )}</span>


Some of these indents look pretty wack. Not sure if that is a prettier bug or some other subtle edge.

I'm not a fan of the line breaks either

pdehaan · 2017-08-25T02:47:02Z

app/templates/file.js

+  }
+
+  function cancel(e) {
+    e.stopPropagation();


Not sure if this should be stopProp(e); or not. Or maybe I'm missing where/how the stopProp() is actually being used, versus being a carryover from older code.

pdehaan · 2017-08-25T02:47:39Z

app/templates/fileList.js

+  <table id="uploaded-files">
+    <thead>
+      <tr>
+        <!-- htmllint attr-bans="false" -->


nit: We don't use htmllint anymore, and can kill these comments. Sorry.

pdehaan · 2017-08-25T02:50:01Z

app/templates/fileList.js

+    </tbody>
+  </table>
+  `;
+  const list = state.storage.files.length ? table : '';


Should we check state.storage.files.length sooner and just abort early if there are no files? May save some l20n lookups and templating, etc.

pdehaan · 2017-08-25T02:54:16Z

app/templates/share.js

+      copyBtn.replaceChild(
+        html`<img src="${assets.get(
+          'check-16.svg'
+        )}" class="icon-check"></img>`,


I know this was just copy/pasta, but we can just use a shorthand class="icon-check" />. Seeing an </img> feels like all sorts of wrong.

pdehaan · 2017-08-25T03:14:30Z

app/utils.js

+    const shim = document.createElement('script');
+    shim.src = polyfill;
+    shim.addEventListener('load', () => resolve(true));
+    shim.addEventListener('error', () => resolve(false));


I can't remember how these are being used, but should we be doing resolve(false) or just rejecting the promise?

It's just a design choice. I'd rather return a boolean than use a try/catch block on the await.

pdehaan · 2017-08-25T17:02:43Z

app/templates/completed.js

+    <div id="download-progress">
+      <div id="dl-title" class="title">${state.translate(
+        'downloadFinish'
+      )}</div>


I'm a bit confused on what the difference between state.translate('blah') and just setting a data-l10n-id="baz" attribute and let l20n handle translations.

The l20n lib is gone. We don't want two different libs both making changes to the same dom nodes. And bonus, this lets us do l10n on the server as well.

ah, OK... in that case, we might have 2 lingering references to data-l10n-id:

$ git grep data-l10n-id app/templates/error.js: <img id="upload-error-img" data-l10n-id="errorAltText" src="${assets.get( app/templates/notFound.js: )}" id="expired-img" data-l10n-id="linkExpiredAlt"/>

ehuggett · 2017-08-28T13:37:59Z

server/routes/index.js

+  app.post('/api/upload', require('./upload'));
+  app.get('/api/download/:id', require('./download'));
+  app.get('/api/exists/:id', require('./exists'));
+  app.post('/api/delete/:id', require('./delete'));


should this* change have incremented the version number? (or if not, could it do so in the future?) I'm not presuming /__version__ will always be available, but i was hoping to use it for the time being as a sanity check ehuggett/send-cli#1

edit: *(the change of path for upload/download/exists/delete)

The version number will change when we tag a release. For the master branch we might want to add a label to the version.json version to indicate that it may not be stable, for example "v1.1.2-master.53e822", meaning the release version will be some semver > 1.1.2. Would that help your situation?

The version number will change when we tag a release.

Its only just dawning on me how foolish attempting to support send.dev.mozaws.net by checking the version number is 🤣 Sorry! (I can't even claim i was half asleep)

It would make the check simpler, but I will check the value of commit AND version to determine this ie if (version == "v1.1.2" && commit == "0bf8481") {ok stable} else {warn about unstable} .
https://send.dev.mozaws.net/__version__
{"commit":"ced640c","source":"https://github.com/mozilla/send/","version":"v1.1.2"}

Which will also work if someone uses the --service-local or --service for dev usage, and might still work on versions >1.1.2 if they are similar enough, so thanks for the suggestion, I didn't realise i already had everything i needed to attempt "reliable" behaviour.

pdehaan reviewed Aug 25, 2017

View reviewed changes

dannycoates force-pushed the choo branch 2 times, most recently from 63179f8 to da82cf7 Compare August 25, 2017 16:35

a few changes to make A/B testing easier

53e8229

dannycoates force-pushed the choo branch from da82cf7 to 53e8229 Compare August 25, 2017 16:45

dannycoates merged commit b684966 into master Aug 25, 2017

pdehaan reviewed Aug 25, 2017

View reviewed changes

ehuggett reviewed Aug 28, 2017

View reviewed changes

dannycoates deleted the choo branch February 22, 2018 19:19

		@@ -0,0 +1,17 @@
		const choo = require('choo');
		const download = require('./download');

Conversation

dannycoates commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehuggett Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehuggett Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ehuggett Aug 28, 2017 •

edited

Loading

ehuggett Aug 28, 2017 •

edited

Loading