Merge pull request #21 from rocicorp/cr

Code review feedback

backend/rds.ts

@@ -103,6 +103,15 @@ async function createDatabase() {
     LastMutationID BIGINT NOT NULL,
     LastModified TIMESTAMP NOT NULL
       DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP)`);
+
+  // For simplicity of demo purposes, and because we don't really need any
+  // advanced backend features, we model backend storage as a kv store. This
+  // allows us to share code more easily and reduces the amount of schema
+  // management goop.
+  //
+  // There's no particular reason that you couldn't use a fully-normalized
+  // relational model on the backend if you want (or need to for legacy)
+  // reasons. Just more work!
   await executeStatement(`CREATE TABLE Object (
     K VARCHAR(100) PRIMARY KEY NOT NULL,
     V TEXT NOT NULL,

frontend/data.ts

@@ -1,8 +1,5 @@
-import Replicache, {
-  JSONValue,
-  ReadTransaction,
-  WriteTransaction,
-} from "replicache";
+import Replicache, { ReadTransaction, WriteTransaction } from "replicache";
+import type { JSONValue } from "replicache";
 import { useSubscribe } from "replicache-react-util";
 import {
   getShape,
@@ -19,7 +16,7 @@ import {
   keyPrefix as clientStatePrefix,
   selectShape,
 } from "../shared/client-state";
-import type Storage from "../shared/storage";
+import type { ReadStorage, WriteStorage } from "../shared/storage";
 import type { UserInfo } from "../shared/client-state";
 import { newID } from "../shared/id";
 
@@ -156,21 +153,15 @@ export function createData(rep: Replicache) {
   };
 }
 
-function readStorage(tx: ReadTransaction): Storage {
+function readStorage(tx: ReadTransaction): ReadStorage {
   return {
-    getObject: tx.get.bind(tx),
-    putObject: () => {
-      throw new Error("Cannot write inside ReadTransaction");
-    },
-    delObject: () => {
-      throw new Error("Cannot delete inside ReadTransaction");
-    },
+    getObject: (key: string) => tx.get(key),
   };
 }
 
-function writeStorage(tx: WriteTransaction): Storage {
+function writeStorage(tx: WriteTransaction): WriteStorage {
   return Object.assign(readStorage(tx), {
-    putObject: tx.put.bind(tx),
-    delObject: tx.del.bind(tx),
+    putObject: (key: string, value: JSONValue) => tx.put(key, value),
+    delObject: async (key: string) => void (await tx.del(key)),
   });
 }

frontend/nav.module.css

@@ -23,10 +23,7 @@
 .user {
   color: white;
   display: flex;
-  margin-left: auto;
-  margin-right: 12px;
-  margin-top: auto;
-  margin-bottom: auto;
+  margin: auto 12px auto auto;
   padding: 0 10px;
   height: 25px;
   align-items: center;

pages/api/replicache-push.ts

@@ -89,6 +89,37 @@ export default async (req: NextApiRequest, res: NextApiResponse) => {
   console.log("Processing push", JSON.stringify(req.body, null, ""));
   const push = must(pushRequest.decode(req.body));
 
+  // Because we are implementing multiplayer, our pushes will tend to have
+  // *lots* of very fine-grained events. Think, for example, of mouse moves.
+  //
+  // I hear you saying, dear reader: "It's silly to send and process all these
+  // little teeny movemove events. Why not collapse consecutive runs of the
+  // same mutation type either on client before sending, or server before
+  // processing.
+  //
+  // We could do that, but there are cases that are more complicated. Consider
+  // drags for example: when a user drags an object, the mutations we get are
+  // like: moveCursor,moveShape,moveCursor,moveShape,etc.
+  //
+  // It's less clear how to collapse sequences like this. In the specific case
+  // of moveCursor/moveShape, you could come up with something that make sense,
+  // but generally Replicache mutations are arbitrary functions of the data
+  // at a moment in time. We can't re-order or collapse them and get a correct
+  // result without re-running them.
+  //
+  // Instead, we take a different tack:
+  // * We send all the mutations, faithfully, from the client (and rely on gzip
+  //   to compress it).
+  // * We open a single, exclusive transaction against MySQL to process all
+  //   mutations in a push.
+  // * We heavily cache (in memory) within that transaction so that we don't
+  //   have to go all the way back to MySQL for each tiny mutation.
+  // * We flush all the mutations to MySQL in parallel at the end.
+  //
+  // As a nice bonus this means that (a) we don't have to have any special-case
+  // collapse logic anywhere, and (b) we get a nice perf boost by parallelizing
+  // the flush at the end.
+
   const t0 = Date.now();
   await transact(async (executor) => {
     const s = storage(executor);

shared/client-state.ts

@@ -1,6 +1,6 @@
 import * as t from "io-ts";
 import { must } from "../backend/decode";
-import Storage from "./storage";
+import { ReadStorage, WriteStorage } from "./storage";
 import { randInt } from "./rand";
 
 const colors = [
@@ -56,7 +56,7 @@ export type UserInfo = t.TypeOf<typeof userInfo>;
 export type ClientState = t.TypeOf<typeof clientState>;
 
 export async function initClientState(
-  storage: Storage,
+  storage: WriteStorage,
   { id, defaultUserInfo }: { id: string; defaultUserInfo: UserInfo }
 ): Promise<void> {
   if (await storage.getObject(key(id))) {
@@ -77,7 +77,7 @@ export async function initClientState(
 }
 
 export async function getClientState(
-  storage: Storage,
+  storage: ReadStorage,
   id: string
 ): Promise<ClientState> {
   const jv = await storage.getObject(key(id));
@@ -88,14 +88,14 @@ export async function getClientState(
 }
 
 export function putClientState(
-  storage: Storage,
+  storage: WriteStorage,
   { id, clientState }: { id: string; clientState: ClientState }
 ): Promise<void> {
   return storage.putObject(key(id), clientState);
 }
 
 export async function setCursor(
-  storage: Storage,
+  storage: WriteStorage,
   { id, x, y }: { id: string; x: number; y: number }
 ): Promise<void> {
   const clientState = await getClientState(storage, id);
@@ -105,7 +105,7 @@ export async function setCursor(
 }
 
 export async function overShape(
-  storage: Storage,
+  storage: WriteStorage,
   { clientID, shapeID }: { clientID: string; shapeID: string }
 ): Promise<void> {
   const client = await getClientState(storage, clientID);
@@ -114,7 +114,7 @@ export async function overShape(
 }
 
 export async function selectShape(
-  storage: Storage,
+  storage: WriteStorage,
   { clientID, shapeID }: { clientID: string; shapeID: string }
 ): Promise<void> {
   const client = await getClientState(storage, clientID);

shared/shape.ts

@@ -1,6 +1,6 @@
 import * as t from "io-ts";
 import { must } from "../backend/decode";
-import Storage from "./storage";
+import { ReadStorage, WriteStorage } from "./storage";
 
 export const shape = t.type({
   type: t.literal("rect"),
@@ -15,7 +15,7 @@ export const shape = t.type({
 export type Shape = t.TypeOf<typeof shape>;
 
 export async function getShape(
-  storage: Storage,
+  storage: ReadStorage,
   id: string
 ): Promise<Shape | null> {
   const jv = await storage.getObject(key(id));
@@ -26,18 +26,18 @@ export async function getShape(
 }
 
 export function putShape(
-  storage: Storage,
+  storage: WriteStorage,
   { id, shape }: { id: string; shape: Shape }
 ): Promise<void> {
   return storage.putObject(key(id), shape);
 }
 
-export function deleteShape(storage: Storage, id: string): Promise<void> {
+export function deleteShape(storage: WriteStorage, id: string): Promise<void> {
   return storage.delObject(key(id));
 }
 
 export async function moveShape(
-  storage: Storage,
+  storage: WriteStorage,
   { id, dx, dy }: { id: string; dx: number; dy: number }
 ): Promise<void> {
   const shape = await getShape(storage, id);

shared/storage.ts

@@ -3,8 +3,11 @@ import { JSONValue } from "replicache";
 /**
  * Interface required of underlying storage.
  */
-export default interface Storage {
+export interface ReadStorage {
   getObject(key: string): Promise<JSONValue | undefined>;
+}
+
+export interface WriteStorage extends ReadStorage {
   putObject(key: string, value: JSONValue): Promise<void>;
   delObject(key: string): Promise<void>;
 }