fix: attribute equality on updates (#470)

.changeset/attribute-equality.md

@@ -0,0 +1,14 @@
+---
+"@empirica/core": patch
+---
+
+Fix setting attributes with the same mutated object as the current one.
+
+For example, before this patch, the value would not be saved, since we are
+reusing the same object, which we've only mutated in place:
+
+```js
+const value = player.get("myobject");
+value["mykey"] = "myvalue";
+player.set("myobject", value);
+```

lib/@empirica/core/src/shared/attributes.ts

@@ -236,6 +236,7 @@ export class Attribute {
   private attrs?: Attribute[];
 
   private val = new BehaviorSubject<JsonValue | undefined>(undefined);
+  private serVal?: string;
 
   constructor(
     private setAttributes: (input: SetAttributeInput[]) => Promise<unknown>,
@@ -294,6 +295,8 @@ export class Attribute {
       throw new Error(`cannot set both append and index`);
     }
 
+    const serVal = JSON.stringify(value);
+
     if (!item && (ao?.index !== undefined || ao?.append)) {
       let index = ao!.index || 0;
       if (ao?.append) {
@@ -316,7 +319,7 @@ export class Attribute {
         );
       } else {
         const existing = this.attrs[index];
-        if (existing && existing.value === value) {
+        if (existing && existing.serVal === serVal) {
           return;
         }
       }
@@ -325,17 +328,19 @@ export class Attribute {
       const v = this._recalcVectorVal();
       this.val.next(v);
     } else {
-      if (this.value === value) {
+      if (this.serVal === serVal) {
         return;
       }
 
       this.val.next(value);
     }
 
+    this.serVal = serVal;
+
     const attrProps: SetAttributeInput = {
       key: this.key,
       nodeID: this.scopeID,
-      val: JSON.stringify(value),
+      val: serVal,
     };
 
     if (ao) {

lib/admin-ui/src/components/batches/NewBatch.svelte

@@ -14,10 +14,10 @@
 </script>
 
 <script>
-  import { formatLobby } from "../../utils/lobbies.js";
   import { push } from "svelte-spa-router";
   import { currentAdmin } from "../../utils/auth.js";
   import { validateBatchConfig } from "../../utils/batches.js";
+  import { formatLobby } from "../../utils/lobbies.js";
   import Alert from "../common/Alert.svelte";
   import ButtonGroup from "../common/ButtonGroup.svelte";
   import EmptyState from "../common/EmptyState.svelte";
@@ -47,7 +47,7 @@
     }
   }
 
-  async function handleSubmit() {
+  async function handleSubmit(event) {
     let config;
     switch (assignmentMethod) {
       case "simple":
@@ -57,20 +57,22 @@
         config = complete;
         break;
       case "custom":
-        config = custom;
+        window["rawCustomConfig"]
+          ? (config = custom.config)
+          : (config = custom);
         break;
       default:
         throw new Error("unknown assgnement method");
     }
 
-    console.info("submitting", JSON.stringify(config));
+    console.debug("submitting", JSON.stringify(config));
 
     const attributes = [
       { key: "config", val: JSON.stringify(config), immutable: true },
       { key: "status", val: JSON.stringify("created"), protected: true },
     ];
 
-    if (assignmentMethod !== "custom") {
+    if (assignmentMethod !== "custom" || window["rawCustomConfig"]) {
       attributes.push({
         key: "lobbyConfig",
         val: JSON.stringify(lobby),
@@ -87,7 +89,7 @@
         attributes,
       });
 
-      console.log("new batch", batch);
+      console.debug("new batch", batch);
       window.lastNewBatch = batch;
 
       newBatch = false;

tests/stress/experiment/client/src/App.jsx

@@ -16,11 +16,43 @@ export default function App() {
       <div className="h-screen relative">
         <EmpiricaMenu position="bottom-left" />
         <div className="h-full overflow-auto">
-          <EmpiricaContext disableConsent>
-            <Game />
+          <EmpiricaContext disableConsent finished={Finished}>
+            <ErrorBoundarySimple>
+              <Game />
+            </ErrorBoundarySimple>
           </EmpiricaContext>
         </div>
       </div>
     </EmpiricaParticipant>
   );
 }
+
+export function Finished() {
+  return (
+    <div>
+      <h2 data-test="game-finished">Finished</h2>
+    </div>
+  );
+}
+
+class ErrorBoundarySimple extends React.Component {
+  state = { hasError: false };
+
+  componentDidCatch(error) {
+    // report the error to your favorite Error Tracking tool (ex: Sentry, Bugsnag)
+    console.error(error);
+  }
+
+  static getDerivedStateFromError(error) {
+    // Update state so the next render will show the fallback UI.
+    return { hasError: true };
+  }
+
+  render() {
+    if (this.state.hasError) {
+      return "Failed";
+    }
+
+    return this.props.children;
+  }
+}

tests/stress/experiment/client/src/Game.jsx

@@ -1,13 +1,58 @@
-import React, { useEffect } from "react";
 import {
   useGame,
   usePlayer,
   usePlayers,
   useRound,
   useStage,
 } from "@empirica/core/player/classic/react";
+import React, { useEffect, useRef } from "react";
 
-export function Game() {
+class Scope {
+  scope = null;
+  keyListeners = new Set();
+  scopeListener = false;
+
+  constructor(kind) {
+    this.kind = kind;
+  }
+
+  updateScope(scope) {
+    this.scope = scope;
+    for (const key of this.keyListeners.keys()) {
+      window.keyChanged(this.kind, key, scope?.get(key), Boolean(scope));
+    }
+
+    if (this.scopeListener) {
+      window.scopeChanged(this.kind, Boolean(scope));
+    }
+  }
+
+  listenKey(key) {
+    this.keyListeners.add(key);
+  }
+
+  listenScope() {
+    this.scopeListener = true;
+  }
+
+  get(key) {
+    return this.scope?.get(key);
+  }
+
+  set(key, value) {
+    this.scope?.set(key, value);
+  }
+}
+const coll = {
+  game: new Scope("game"),
+  round: new Scope("round"),
+  stage: new Scope("stage"),
+  player: new Scope("player"),
+  players: new Scope("players"),
+};
+window.empirica_test_collector = coll;
+
+function GameInner() {
   const game = useGame();
   const round = useRound();
   const stage = useStage();
@@ -20,37 +65,62 @@ export function Game() {
   }, []);
 
   useEffect(() => {
-    window.game = game;
-    window.round = round;
-    window.stage = stage;
-    window.player = player;
-    window.players = players;
-
-    return () => {
-      delete window.game;
-      delete window.round;
-      delete window.stage;
-      delete window.player;
-      delete window.players;
-    };
-  }, []);
+    coll.game.updateScope(game);
+  }, [game]);
+
+  useEffect(() => {
+    coll.round.updateScope(round);
+  }, [round]);
+
+  useEffect(() => {
+    coll.stage.updateScope(stage);
+  }, [stage]);
 
-  // useEffect(() => {
-  //   let i = 0;
-  //   const interval = setInterval(() => {
-  //     round.set(`someKey ${i}`, "someValue");
-  //     i++;
-  //   }, 1000 / treatment.newKeyRate);
+  useEffect(() => {
+    coll.player.updateScope(player);
+  }, [player]);
+
+  useEffect(() => {
+    coll.players.updateScope(players);
+  }, [players]);
 
-  //   return () => clearInterval(interval);
-  // }, []);
+  console.log("submitted", player.stage.get("submit"));
 
   return (
     <div>
       <h1 data-test="game-started">Game started</h1>
       <h2 data-test="round-name">{round.get("name")}</h2>
       <h3 data-test="stage-name">{stage.get("name")}</h3>
       <h3 data-test="player-count">{treatment.playerCount}</h3>
+
+      {player.stage.get("submit") ? (
+        <div data-test="submitted">Submitted</div>
+      ) : (
+        <div data-test="stage-ongoing">Stage ongoing</div>
+      )}
+
+      <button
+        data-test="submit-stage"
+        onClick={() => {
+          player.stage.set("submit", true);
+        }}
+      >
+        Submit
+      </button>
     </div>
   );
 }
+
+export class Game extends React.Component {
+  componentWillUnmount() {
+    coll.game.updateScope(null);
+    coll.round.updateScope(null);
+    coll.stage.updateScope(null);
+    coll.player.updateScope(null);
+    coll.players.updateScope(null);
+  }
+
+  render() {
+    return <GameInner />;
+  }
+}

tests/stress/playwright.config.js

@@ -20,7 +20,9 @@ module.exports = defineConfig({
   /* Retry on CI only */
   retries: process.env.CI ? 2 : 0,
   /* Opt out of parallel tests on CI. */
-  workers: process.env.CI ? 1 : undefined,
+  // workers: process.env.CI ? 1 : undefined,
+  /* Opt out of parallel tests for now, since using a single external empirica instance. */
+  workers: 1,
   /* Reporter to use. See https://playwright.dev/docs/test-reporters */
   reporter: [["html", { open: "never" }]],
   /* Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions. */