Rewrite frontend in React #41

mythmon · 2018-10-06T23:20:49Z

This needs a few more features, a clean up pass, docs, etc. Put I thought it is interesting enough to start sharing, if you want to take a look.

I'll write docs for this, and make it generally easy to deploy, but for now the quick version is to install the dependencies with Yarn (yarn install), run the Python app with ./app.py, and separately run CRA with yarn start.

peterbe

I'm looking forward too take the time to play with this. I just did a drive-by look but haven't done a deep dig yet.
Is it ready for that?

peterbe · 2018-10-09T00:44:59Z

public/index.html

+<html lang="en">
+  <head>
+    <meta charset="utf-8">
+    <title>What's deployed? (React)</title>


Let's not forget to remove the (React) part.

Oh definitely. This is here because during testing I've had both this and the prod version open at once and flip back and forth, and I kept getting confused which one I was looking at. 😆

peterbe · 2018-10-09T00:50:03Z

src/SetupPage.js

+        </p>
+        <div>
+          <button className="btn btn-primary">
+            Generate <i>What's Deployed</i> Table


Why is GitHub doing this?!

Github doesn't really understand JSX, and so it thinks this single quote is the start of an unterminated string. Frustrating

Yeah, but it's not the first time I've used ' characters in GitHub PRs. Just got me worried that we're doing something wrong this time.

For the record, here's the open issue to resolve this:
github-linguist/linguist#3677

Relax. I'lll fix it, eventually. I started but something more important kinda stole amy attention for a little while...

The grammar I began work on would aim to be THE only ES grammar you'd need, supporting ESX and Flow Static Type Hintings, and whatnot... basically, it could be the one JS grammar you could use everywhere and not have to forget about subtle discrepancies between Atom and VSCode's pattern-handling, of that of Sublime Text iand so forth...

peterbe · 2018-10-09T00:53:04Z

src/registerServiceWorker.js

@@ -0,0 +1,117 @@
+// In production, we register a service worker to serve assets from local cache.


This file is not the latest and greatest from create-react-app.

Hm. I started with CRA 1.x and followed the instructions for migrating to 2.0. I'll see if I can do better.

I don't think the serviceworker gets upgrades when you upgrade because the file is part of the src directory and dumped from the template into the new directory.

The new hotness (and it's hot!) is called serviceWorker.js and the only way to get it, as far as I know, is to create a throw-away cra app in /tmp and copy the src/serviceWorker.js file from there. And then to also add...:

import * as serviceWorker from './serviceWorker';

and at the bottom:

serviceWorker.register({ onUpdate: () => { window.location.reload() } });

mythmon · 2018-10-29T00:59:16Z

@peterbe I think this is ready to go now. I gave it a quick once over, but it could definitely do with more eyes. I also don't really know how you deploy this. The docker-compose set up works for me locally. Let me know if that doesn't gel well with what you use now.

peterbe

I'm not entirely sure I care but I did notice that apt-get install -y nodejs yarn causes this...

...
Reading package lists...
Building dependency tree...
Reading state information...
The following additional packages will be installed:
  bzip2 file libmagic-mgc libmagic1 libpython-stdlib libpython2.7-minimal
  libpython2.7-stdlib mime-support python python-minimal python2.7
  python2.7-minimal xz-utils
Suggested packages:
  bzip2-doc python-doc python-tk python2.7-doc binfmt-support
The following NEW packages will be installed:
  bzip2 file libmagic-mgc libmagic1 libpython-stdlib libpython2.7-minimal
  libpython2.7-stdlib mime-support nodejs python python-minimal python2.7
  python2.7-minimal xz-utils yarn
0 upgraded, 15 newly installed, 0 to remove and 1 not upgraded.
Need to get 20.8 MB of archives.
After this operation, 99.2 MB of additional disk space will be used.
...

I think a better approach is to use something like https://github.com/mozilla-services/tecken/blob/master/Dockerfile#L63 instead.

But, like I said, I'm not sure it matters. The whatsdeployed_web image is 688MB which isn't a sneeze.

Either way, we can park it and iterate.

When you worked on this, did you actively use Docker or did you use two terminals (with one in virtualenv)?

peterbe · 2018-11-01T00:53:06Z

README.md

@@ -30,7 +30,7 @@ Development
 -----------

 You can either do development with Docker (recommended) or with a plain


I don't even use Docker locally when I do dev. :)

peterbe · 2018-11-01T01:07:26Z

Any ideas what these mean?

▶ docker-compose up web
...
web_1         | yarn run v1.12.1
web_1         | warning Skipping preferred cache folder "/home/app/.cache/yarn" because it is not writable.
web_1         | warning Selected the next writable cache folder in the list, will be "/tmp/.yarn-cache-10001".
web_1         | $ react-scripts start
web_1         | warning Cannot find a suitable global folder. Tried these: "/usr/local, /home/app/.yarn"
...

peterbe · 2018-11-01T01:12:48Z

Pressing the Add Row button doesn't do anything.

peterbe

It's late but I poke around a bit. I never got around to actually running it yet when I discovered that the add form doesn't work. I'll get around to more testing tomorrow hopefully. Left some low-hanging fruit fixes in the meantime.

peterbe · 2018-11-01T01:09:01Z

src/SetupPage.js

+          </button>
+        </div>
+
+        <div id="previous" style={{ display: 'none' }}>


I think we can delete this now, no?

peterbe · 2018-11-01T01:10:37Z

src/SetupPage.js

+          </li>
+        ))}
+      </ul>
+    </div>


Can you refactor these blocks so that we don't have to repeat the <div id="previous"> and the <h3> tag.

By the way, can you fix it so it says "Previous Environments" (instead of "PreviousEnvironments").

peterbe · 2018-11-01T01:14:15Z

src/SetupPage.js

+          />
+        </div>
+        <p>
+          <button type="button" className="btn btn-secondary more">


This needs an onClick handler which triggers the display of another row.

peterbe · 2018-11-01T01:14:39Z

src/SetupPage.js

+          </button>
+        </p>
+        <div>
+          <button className="btn btn-primary">


This also needs an onClick handler that actually starts the redirect.

peterbe · 2018-11-01T01:16:29Z

src/SetupPage.js

+            placeholder="e.g. airmozilla"
+          />
+        </div>
+        <div className="form-group revisions">


This whole block needs to be a loop over something like this.state.rows.

peterbe · 2018-11-01T01:43:41Z

src/shortUrls.js

+      let idx = history.indexOf(shortUrl);
+      if (idx !== -1) {
+        history.splice(idx, 1);
+      }


The lines above can be simplified to:

history = history.filter(url => url !== shortUrl);

peterbe · 2018-11-01T01:52:35Z

src/shortUrls.js

+import ky from 'ky';
+
+let maxHistory = 5;
+let history;


Leave it as a array all the time. E.g.

const history = [];

and

history.length = 0; history.concat(JSON.parse(json));

and when resetting it just do

history.length = 0;

and instead of if (!history) { or if (history.length > 0) { use if (!history.length) { and if (history.length) { respectively.

I did not realize you could truncate an array by setting its length.

peterbe · 2018-11-01T01:55:54Z

src/DeployPage.js

+      loading: null,
+      tags: null
+    };
+  }


As mentioned elsewhere we can just do:

state = { commits: null, deployInfo: null, error: null, loading: null, tags: null };

peterbe · 2018-11-01T02:04:52Z

src/DeployPage.js

+    const { owner, repo } = this.props;
+    const { error, loading, deployInfo, commits, tags } = this.state;
+
+    document.title = `What's deployed on ${owner}/${repo}?`;


nit: "Deployed", not "deployed". Also, the trailing question marks is supposed to disappear based on it now being known. So the question mark should change depending on this.isLoading().

peterbe · 2018-11-01T02:08:34Z

src/DeployPage.js

+    if (this.isLoading()) {
+      return (
+        <div>
+          <span>Loading {Array.from(loading || '...').join(' and ')}</span>


What does this do? If loading is null the output of this is <span>. and . and .</span>.

I think that it would actually be an error, since '...'.join(' and ') throws TypeError: "...".join is not a function. This is a bug.

peterbe · 2018-11-01T02:13:02Z

It's your branch but after having built a lot of React apps (without Typescript or Flow) one thing that repeatedly has saved me is to stick to the same types and not switch between null and {} or between null and [].

In my older code, I used to do this.state = {something: null} and then later switch to this.setState({something: ['an', 'array', 'for', 'example']}) and those would sooner or later lead to either TypeErrors or bugs related to checking their truthyness.

mythmon · 2018-11-01T20:58:47Z

I think a better approach is to use something like https://github.com/mozilla-services/tecken/blob/master/Dockerfile#L63 instead.

But, like I said, I'm not sure it matters. The whatsdeployed_web image is 688MB which isn't a sneeze.

Either way, we can park it and iterate.

I agree, and actually have another patch ready to do just that, but I figured it would be better to save for another PR. I could bring it into this PR if you like.

When you worked on this, did you actively use Docker or did you use two terminals (with one in virtualenv)?

I used two terminals most of the time, but did a little bit of work in Docker at the end just to test it out.

It's your branch but after having built a lot of React apps (without Typescript or Flow) one thing that repeatedly has saved me is to stick to the same types and not switch between null and {} or between null and [].

In my older code, I used to do this.state = {something: null} and then later switch to this.setState({something: ['an', 'array', 'for', 'example']}) and those would sooner or later lead to either TypeErrors or bugs related to checking their truthyness.

Funny, usually I'm the one advocating for variables not changing types. You're totally right. I'll make a pass and tweak that kind of stuff.

mythmon · 2018-11-01T21:00:11Z

Oh, and the errors in the docker-compose output are because the Docker file is effectively readonly, and apparently Yarn isn't happy about that. I'll see if I can suppress it or make it better somehow.

peterbe · 2019-01-28T19:04:13Z

src/App.js

@@ -0,0 +1,61 @@
+import React from 'react';
+import 'bootstrap';
+import 'bootstrap/dist/css/bootstrap.min.css';


This is repeated in index.js. Can we just keep this stuff and make index.js pure of CSS?

peterbe · 2019-01-28T19:11:34Z

src/App.js

+          deployments={deployments}
+        />
+      );
+    }


We need a solution to cope with loading shortlinks. When you go to http://localhost:3000/s-xXq it used to redirect to http://localhost:3000/?name%5B%5D=Dev&name%5B%5D=Stage&name%5B%5D=Prod&owner=mozilla-services&url%5B%5D=https%3A%2F%2Fsymbols.dev.mozaws.net%2F__version__&url%5B%5D=https%3A%2F%2Fsymbols.stage.mozaws.net%2F__version__&url%5B%5D=https%3A%2F%2Fsymbols.prod.mozaws.net%2F__version__&repo=tecken

I'm started to doubt the redirect, but we definitely need to make this page work. What it ought too do is load a component that wraps the DeployPage component, but upon mount it should do an XHR to get all the info that that shortlink represents. Perhaps, with that in place, we can just stay on http://localhost:3000/s-xXq without a redirect.

I think the redirect was there because it was bolted on as an after-thought and it'd just basically help you fill in the fields in the setup form.
Yeah, the more I think about it, let's not do a redirect.

But, if you're on http://localhost:3000/s-xXq and click the header ("What's Deployed") perhaps it can go to http://localhost:3000/ so the user can get back to the true home page.

peterbe · 2019-01-28T19:15:57Z

src/App.js

+  render() {
+    let content = <SetupPage />;
+
+    if (window.location.search) {


Perhaps this should "die". Perhaps, what this should do is, send the deployments, owner, and repo to the backend to get-or-create the shortlink and then redirect to that.

peterbe reviewed Oct 9, 2018

View reviewed changes

mythmon force-pushed the react branch 2 times, most recently from 90c568a to 6ed1618 Compare October 29, 2018 00:54

Rewrite the frontend in React

6cea01e

mythmon force-pushed the react branch from 6ed1618 to 6cea01e Compare October 29, 2018 00:55

mythmon changed the title ~~[WIP] Rewrite frontend in React~~ Rewrite frontend in React Oct 29, 2018

mythmon added 2 commits October 28, 2018 18:40

In docker-compose, mount more precisely for more reliable dev setup

14da2fb

Remove a couple stray "(React)"s

fc11a4c

peterbe reviewed Nov 1, 2018

View reviewed changes

Add routing and make setup form work

918b835

peterbe reviewed Jan 28, 2019

View reviewed changes

peterbe merged commit 77f3cdd into peterbe:master Jan 28, 2019

		@@ -0,0 +1,117 @@
		// In production, we register a service worker to serve assets from local cache.

		@@ -30,7 +30,7 @@ Development
		-----------

		You can either do development with Docker (recommended) or with a plain

Rewrite frontend in React #41

Rewrite frontend in React #41

Conversation

mythmon commented Oct 6, 2018

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mythmon commented Oct 29, 2018

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe commented Nov 1, 2018

peterbe commented Nov 1, 2018

peterbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbe commented Nov 1, 2018

mythmon commented Nov 1, 2018

mythmon commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment