Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite frontend in React #41

Merged
merged 4 commits into from Jan 28, 2019

Conversation

Projects
None yet
3 participants
@mythmon
Copy link
Collaborator

mythmon commented Oct 6, 2018

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
Copy link
Owner

peterbe left a comment

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?

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

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 9, 2018

Owner

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

This comment has been minimized.

Copy link
@mythmon

mythmon Oct 9, 2018

Author Collaborator

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. 😆

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 10, 2018

Owner

Smart!

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

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 9, 2018

Owner

Why is GitHub doing this?!

This comment has been minimized.

Copy link
@mythmon

mythmon Oct 9, 2018

Author Collaborator

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

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 10, 2018

Owner

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.

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

This comment has been minimized.

Copy link
@Alhadis

Alhadis Nov 1, 2018

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...

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

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 9, 2018

Owner

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

This comment has been minimized.

Copy link
@mythmon

mythmon Oct 9, 2018

Author Collaborator

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

This comment has been minimized.

Copy link
@mythmon

mythmon Oct 9, 2018

Author Collaborator

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

This comment has been minimized.

Copy link
@peterbe

peterbe Oct 10, 2018

Owner

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 mythmon force-pushed the mythmon:react branch 2 times, most recently from 90c568a to 6ed1618 Oct 29, 2018

@mythmon mythmon force-pushed the mythmon:react branch from 6ed1618 to 6cea01e Oct 29, 2018

@mythmon

This comment has been minimized.

Copy link
Collaborator Author

mythmon commented Oct 29, 2018

@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.

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

mythmon added some commits Oct 29, 2018

@peterbe
Copy link
Owner

peterbe left a comment

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)?

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

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

@peterbe

This comment has been minimized.

Copy link
Owner

peterbe commented Nov 1, 2018

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

This comment has been minimized.

Copy link
Owner

peterbe commented Nov 1, 2018

Pressing the Add Row button doesn't do anything.
screen shot 2018-10-31 at 9 11 33 pm

@peterbe
Copy link
Owner

peterbe left a comment

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.

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

I think we can delete this now, no?

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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").

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

The lines above can be simplified to:

history = history.filter(url => url !== shortUrl);
import ky from 'ky';

let maxHistory = 5;
let history;

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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.

This comment has been minimized.

Copy link
@mythmon

mythmon Nov 1, 2018

Author Collaborator

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

loading: null,
tags: null
};
}

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

As mentioned elsewhere we can just do:

state = {
  commits: null,
  deployInfo: null,
  error: null,
  loading: null,
  tags: null
};
const { owner, repo } = this.props;
const { error, loading, deployInfo, commits, tags } = this.state;

document.title = `What's deployed on ${owner}/${repo}?`;

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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().

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

This comment has been minimized.

Copy link
@peterbe

peterbe Nov 1, 2018

Owner

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

This comment has been minimized.

Copy link
@mythmon

mythmon Nov 1, 2018

Author Collaborator

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

@peterbe

This comment has been minimized.

Copy link
Owner

peterbe commented Nov 1, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

mythmon commented Nov 1, 2018

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

This comment has been minimized.

Copy link
Collaborator Author

mythmon commented Nov 1, 2018

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.

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

This comment has been minimized.

Copy link
@peterbe

peterbe Jan 28, 2019

Owner

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

deployments={deployments}
/>
);
}

This comment has been minimized.

Copy link
@peterbe

peterbe Jan 28, 2019

Owner

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.

This comment has been minimized.

Copy link
@peterbe

peterbe Jan 28, 2019

Owner

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.

render() {
let content = <SetupPage />;

if (window.location.search) {

This comment has been minimized.

Copy link
@peterbe

peterbe Jan 28, 2019

Owner

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 peterbe merged commit 77f3cdd into peterbe:master Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.