notebook cleanups #21

qti3e · 2018-05-12T21:30:47Z

TODO:

ry

Looks good! Let me know when I should review again.

ry · 2018-05-12T23:39:48Z

src/types.ts

+
+/**
+ * This file contains common types we use across the project.
+ */


Nit: 1 line instead of 3

// Common types used across the project.

ry · 2018-05-12T23:42:38Z

src/snippet.tsx

+ */
+
+/**
+ * Snippet is a cell showsing preview of a notebook.


typo "showsing"

Also - this could use a better name - like "NotebookPreview" or something.

ry · 2018-05-12T23:45:03Z

src/list.tsx

+  onClick?: (nbId: string) => void;
+}
+
+export class List extends Component<ListProps, {}> {


maybe call it NotebookList?

ry · 2018-05-12T23:46:17Z

src/profile.tsx

+    const doc = this.props.notebooks[0].doc;
+    // TODO Profile is reusing the most-recent css class, because it's a very
+    // similar layout. The CSS class should be renamed something appropriate
+    // for both of them, maybe nb-listing.


Good TODO. Let's try to move towards consistency between the CSS classes and the component names.

qti3e · 2018-05-14T18:41:00Z

Waiting for preactjs/preact-router#281 to be resolved...

piscisaureus · 2018-05-14T19:01:38Z

src/components/logo.tsx

+   limitations under the License.
+ */
+
+// tslint:disable:variable-name


I would suggest to use tslint:disable-next-line:variable-name and move it where it's needed.

piscisaureus · 2018-05-14T19:02:34Z

src/components/index.tsx


 export const PropelIndex = props => {
-  let md = readFileSync(__dirname + "/../README.md", "utf8");
+  console.log(__dirname);


leftover debug statement?

removed, thank you Bert

piscisaureus · 2018-05-15T22:03:51Z

@qti3e

I landed some of your commits (including moving components to src/components).
I rebased the other patches you made and updated this pull request. The original branch is still there as q/cleanup_backup, should you need it (but the diff is minimal).

qti3e · 2018-05-18T08:09:43Z

@ry @piscisaureus PTAL

ry · 2018-05-18T14:59:02Z

@qti3e This branch needs to be rebased

ry · 2018-05-18T15:01:57Z

src/app.tsx

+  error: string;
+}
+
+function bind(C, bindProps: BindProps) {


Is this a common react pattern? Can you add docs or a link to a description?

@ry https://reactjs.org/docs/higher-order-components.html

ok - Please add that link to the source code

src/app.tsx

+      this.loadData();
+      const { data, error } = this.state;
+      if (error) return <ErrorPage message={error} />;
+      if (!data) return <Loading />;


ry · 2018-05-18T15:06:44Z

src/app.tsx

+      <div class="notebook">
+        <GlobalHeader subtitle="Notebook" subtitleLink="/">
+          <UserMenu userInfo={userInfo} />
+        </GlobalHeader>


Nice that we have a single reference to the global header now.

piscisaureus · 2018-05-18T17:25:19Z

@qti3e https://gist.github.com/fffc74c4dbbf65e275d3093d043ab8fc <-- conflicting changes

piscisaureus · 2018-05-18T19:34:41Z

src/components/common.tsx

+  // Careful, S3 is finicky about what URLs it serves. So
+  // /notebook?nbId=blah  will get redirect to /notebook/
+  // because it is a directory with an index.html in it.
+  const u = window.location.origin + "/notebook/?nbId=" + nbId;


This needs to be rebased.

piscisaureus · 2018-05-18T19:37:42Z

src/app.tsx

+  created: new Date(),
+  owner: {
+    displayName: "Anonymous",
+    photoURL: "/static/img/anon_profile.png?",


This needs to be rebased

piscisaureus · 2018-05-18T21:08:49Z

tsconfig.json

@@ -19,6 +19,7 @@
    "preserveConstEnums": true,
    "pretty": true,
    "sourceMap": true,
+    "allowSyntheticDefaultImports": true,


Why is this needed?

import pathToRegexp from "path-to-regexp";

the default export of path-to-regexp is a function and we need to use that, on the other hand,when allowSyntheticDefaultImports is disabled, ts will throw an error:

"path-to-regexp" has no default export.

Did you try import * as pathToRegExp from "path-to-regexp" ?

@piscisaureus yeah, but it would throw a run-time error (see my comment below)

piscisaureus · 2018-05-18T21:27:28Z

@qti3e I rebased this branch again.
But made no changes to the router.

qti3e · 2018-05-19T15:03:12Z

@piscisaureus @ry I made some changes to the router and implemented a new component (<Link />) so we can deploy on GitHub pages, I think this patch is ready to land now : )

piscisaureus · 2018-05-20T04:23:52Z

src/app.tsx

+import { Recent } from "./components/recent";
+
+interface BindProps {
+  [key: string]: (props: any) => Promise<any>;


The issue I have with this is that it breaks the type-checker.

I can somewhat bring back type safety by applying the following patch:

diff --git a/src/app.tsx b/src/app.tsx index 85d3297..318aa28 100644 --- a/src/app.tsx +++ b/src/app.tsx @@ -27,15 +27,26 @@ import { Notebook } from "./components/notebook"; import { Profile } from "./components/profile"; import { Recent } from "./components/recent"; -interface BindProps { - [key: string]: (props: any) => Promise<any>; +type Partial<T> = { + [K in keyof T]?: T[K]; +}; + +type BindProps = { + readonly [K in keyof P]?: (props: P) => Promise<P[K]>; +}; + +interface BindStateNormal { + data: { [K in keyof P]: P[K] }; + error: null; } -interface BindState { - data: { [key: string]: string }; +interface BindStateError { + data: null; error: string; } +type BindState = BindStateNormal | BindStateError; + /** * This react HOC can be used to bind result of some async * methods to props of the given component (C). @@ -48,8 +59,8 @@ interface BindState { * } * }); */ -function bind(C, bindProps: BindProps) { - return class extends Component<any, BindState> { +function bind(C, bindProps: BindProps) { + return class extends Component<Partial, BindState> { state = { data: null, error: null }; prevMatches = null; componentRef; @@ -61,7 +72,7 @@ function bind(C, bindProps: BindProps) { async loadData() { if (equal(this.props.matches, this.prevMatches)) return; this.prevMatches = this.props.matches; - const data = {}; + const data: any = {}; for (const key in bindProps) { if (!bindProps[key]) continue; try {

However typescript complains about the fact that the onReady and matches properties aren't defined anywhere, and it looks like it's right about that...
Same for the path attribute used when instantiating the page components (<RecentPage path="/" userInfo={userInfo} /> etc.)

I am not afraid of some black magic voodoo code, but it has to be correct before I am going to accept it.

On an aside, did you consider simply using getters/setters? Maybe that's a bit easier to reason about.

piscisaureus · 2018-05-20T06:33:50Z

@qti3e See c519b5c for my attempt to make bind() type safe. I hope we can do something that's a little easier to understand.

qti3e · 2018-05-20T13:04:36Z

@piscisaureus I dropped 1e67a10 due to:

TypeError: pathToRegexp is not a function
    at match (db.ts:312)
    at Router.onLocationChange (db.ts:312)
    at Router.componentWillMount (db.ts:312)
    at setComponentProps (test.b1fdbaba.js:1545)
    at buildComponentFromVNode (test.b1fdbaba.js:1742)
    at idiff (test.b1fdbaba.js:1283)
    at diff (test.b1fdbaba.js:1234)
    at Object.render (test.b1fdbaba.js:1875)
    at router (router_test.tsx:24)
    at runTests (tester.ts:104)

qti3e · 2018-05-21T11:49:14Z

@piscisaureus PTAL

ry

LGTM - cursory review - I defer to @piscisaureus for the final approval

src/app.tsx

+      this.loadData();
+      const { data, error } = this.state;
+      if (error) return <ErrorPage message={error} />;
+      if (!data) return <Loading />;


ry · 2018-05-21T13:12:33Z

src/app.tsx

+}
+
+// An anonymous notebook doc for when users aren't logged in
+const anonDoc = {


Should this be exported so it could be used in tests?

ry · 2018-05-21T13:16:40Z

src/router_test.tsx

+  await flush();
+  assertEqual(path(), "/page/345");
+  assertEqual(activePage(), "p3");
+  assertEqual(matchedProps.get("p3").v, "345");


ry · 2018-05-21T13:20:41Z

src/components/error.tsx

+  message: string;
+}
+
+export function ErrorPage(props: ErrorPageProps) {


Nit: This could be done in one line...

function ErrorPage(props: { header?: string, message: string }): JSX.Element {

max-line-length: Exceeds maximum line length of 80

qti3e · 2018-05-23T19:15:32Z

@piscisaureus any comments? : )

piscisaureus · 2018-05-29T23:03:47Z

src/app.tsx

+    }
+
+    render() {
+      this.loadData();


I'm sorry but this just isn't right.
Since loadData() is async, it causes the following loop:

tick 1: render() -> starts loadData()
tick 2: loadData() calls setState() -> render() gets called -> starts another loadData().

@piscisaureus loadData only calls setState when props has been changed

+ if (equal(this.props.matches, this.prevMatches)) return; + this.prevMatches = this.props.matches;

piscisaureus · 2018-05-29T23:05:26Z

src/app.tsx

+  }
+});
+
+export const HomePage = bind(Home as any, {});


as any is inappropriate here.

that's because Home is a functional component... I'll try again to remove this any

and other unused codes

and some fixes in components

qti3e requested review from ry and piscisaureus May 12, 2018 21:30

ry reviewed May 12, 2018

View reviewed changes

qti3e force-pushed the q/cleanup branch 3 times, most recently from 55feba0 to b3ff2fe Compare May 13, 2018 17:14

piscisaureus reviewed May 14, 2018

View reviewed changes

qti3e force-pushed the q/cleanup branch 3 times, most recently from 1a7b490 to ab76944 Compare May 15, 2018 16:38

piscisaureus force-pushed the q/cleanup branch from ab76944 to f3a4ff2 Compare May 15, 2018 22:01

qti3e force-pushed the q/cleanup branch 4 times, most recently from 64ecd69 to 2ea7e8b Compare May 18, 2018 07:58

qti3e changed the title ~~[WIP] notebook clean ups~~ notebook cleanups May 18, 2018

ry reviewed May 18, 2018

View reviewed changes

qti3e force-pushed the q/cleanup branch from 2ea7e8b to 5992776 Compare May 18, 2018 17:40

piscisaureus reviewed May 18, 2018

View reviewed changes

piscisaureus force-pushed the q/cleanup branch from 5992776 to a22fafd Compare May 18, 2018 21:26

qti3e force-pushed the q/cleanup branch from a22fafd to 3b000d6 Compare May 19, 2018 14:57

piscisaureus force-pushed the q/cleanup branch from cf77a3d to ba5b9cd Compare May 20, 2018 04:21

piscisaureus reviewed May 20, 2018

View reviewed changes

piscisaureus force-pushed the q/cleanup branch 4 times, most recently from 7332616 to c519b5c Compare May 20, 2018 06:31

qti3e force-pushed the q/cleanup branch from c519b5c to 4b0dedb Compare May 20, 2018 13:01

qti3e force-pushed the q/cleanup branch from 4b0dedb to 360fda4 Compare May 21, 2018 11:44

qti3e force-pushed the q/cleanup branch 3 times, most recently from b1c80a2 to 2d5018d Compare May 21, 2018 13:21

ry reviewed May 21, 2018

View reviewed changes

piscisaureus reviewed May 29, 2018

View reviewed changes

qti3e mentioned this pull request Jun 3, 2018

Websocket backend #40

Open

qti3e and others added 10 commits June 3, 2018 20:15

components: ErrorPage

1518268

Implement router

c21bd29

rm notebook_root.tsx

7aa2f6e

and other unused codes

Implement Propel router

f452ad0

and some fixes in components

Update nb_test

f17a235

Use hash history instead of browser's native history

0af2e48

Make router type-safe

04f669c

Write tests for router

9df964f

Remove GlobalHeader from home page

99e700b

Reviews

87e8e31

qti3e force-pushed the q/cleanup branch from 49b82c8 to 87e8e31 Compare June 3, 2018 15:46

notebook cleanups #21

Are you sure you want to change the base?

notebook cleanups #21

Conversation

qti3e commented May 12, 2018 • edited Loading

ry 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

qti3e commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qti3e May 15, 2018 • edited Loading

Choose a reason for hiding this comment

piscisaureus commented May 15, 2018

qti3e commented May 18, 2018

ry commented May 18, 2018

Choose a reason for hiding this comment

qti3e May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented May 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented May 18, 2018

qti3e commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piscisaureus commented May 20, 2018

qti3e commented May 20, 2018

qti3e commented May 21, 2018

ry left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

qti3e commented May 23, 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

qti3e commented May 12, 2018 •

edited

Loading

qti3e commented May 14, 2018 •

edited

Loading

qti3e May 15, 2018 •

edited

Loading

qti3e May 18, 2018 •

edited

Loading