🐛 Fix lost writes and failed loads in the storage adapters

src/lib/state/persistence/adapters/createLocalStorageAdapter.test.ts

@@ -0,0 +1,47 @@
+import { afterEach, describe, expect, it } from "vitest"
+import { createLocalStorageAdapter } from "./createLocalStorageAdapter"
+
+afterEach(() => {
+  localStorage.clear()
+})
+
+describe("createLocalStorageAdapter", () => {
+  it("should read a value that is not JSON as nothing stored", async () => {
+    localStorage.setItem("theme", "dark")
+
+    expect(await createLocalStorageAdapter().getItem("theme")).toBe(undefined)
+  })
+
+  describe("with a shared key", () => {
+    it("should keep both of two overlapping setItem calls", async () => {
+      const adapter = createLocalStorageAdapter({ key: "app" })
+
+      await Promise.all([adapter.setItem("a", 1), adapter.setItem("b", 2)])
+
+      expect(JSON.parse(localStorage.getItem("app") ?? "")).toEqual({
+        a: 1,
+        b: 2,
+      })
+    })
+
+    it("should apply both of an overlapping setItem and removeItem", async () => {
+      const adapter = createLocalStorageAdapter({ key: "app" })
+      await adapter.setItem("a", 1)
+
+      await Promise.all([adapter.setItem("b", 2), adapter.removeItem("a")])
+
+      expect(JSON.parse(localStorage.getItem("app") ?? "")).toEqual({ b: 2 })
+    })
+
+    it("should read a shared entry that is not JSON as an empty store", async () => {
+      localStorage.setItem("app", "not json")
+      const adapter = createLocalStorageAdapter({ key: "app" })
+
+      expect(await adapter.getItem("theme")).toBe(undefined)
+
+      await adapter.setItem("theme", "dark")
+
+      expect(await adapter.getItem("theme")).toBe("dark")
+    })
+  })
+})

src/lib/state/persistence/adapters/createLocalStorageAdapter.ts

@@ -1,54 +1,48 @@
 import type { Adapter } from "./Adapter"
+import { parseJson } from "./parseJson"
 
-const normalizeStore = (store: unknown) => {
-  if (!store || typeof store !== "object") {
-    return undefined
-  }
+type Store = Record<string, unknown>
+
+const isStore = (value: unknown): value is Store =>
+  typeof value === "object" && value !== null
+
+const readStore = (key: string): Store => {
+  const store = parseJson(localStorage.getItem(key))
 
-  return store
+  return isStore(store) ? store : {}
+}
+
+const writeStore = (key: string, store: Store) => {
+  localStorage.setItem(key, JSON.stringify(store))
 }
 
 /**
  * Create an adapter which use one unique store entry to store all
  * state. When using many signals it can help with maintenance to keep things
  * tidy.
+ *
+ * Each call reads and writes the entry synchronously, with no `await` in
+ * between, so two calls that overlap do not lose a write.
  */
-const createSharedStoreAdapter = ({
-  adapter,
-  key,
-}: {
-  adapter: Adapter
-  key: string
-}): Adapter => ({
+const createSharedStoreAdapter = (key: string): Adapter => ({
   clear: async () => {
-    return await adapter.removeItem(key)
+    localStorage.removeItem(key)
   },
 
   removeItem: async (keyToDelete) => {
-    const unsafeStore = await adapter.getItem(key)
-    const { [keyToDelete]: toRemove, ...rest } =
-      // biome-ignore lint/suspicious/noExplicitAny: TODO
-      normalizeStore(unsafeStore) ?? ({} as any)
+    const { [keyToDelete]: _removed, ...rest } = readStore(key)
 
-    await adapter.setItem(key, rest)
+    writeStore(key, rest)
   },
 
   getItem: async (itemKey: string) => {
-    const unsafeStore = await adapter.getItem(key)
-    const store = normalizeStore(unsafeStore) ?? {}
-
-    if (itemKey in store) {
-      return store[itemKey as keyof typeof store]
-    }
+    const store = readStore(key)
 
-    return undefined
+    return Object.hasOwn(store, itemKey) ? store[itemKey] : undefined
   },
 
   setItem: async (itemKey: string, value: unknown) => {
-    const unsafeStore = await adapter.getItem(key)
-    const store = normalizeStore(unsafeStore) ?? {}
-
-    await adapter.setItem(key, { ...store, [itemKey]: value })
+    writeStore(key, { ...readStore(key), [itemKey]: value })
   },
 })
 
@@ -58,10 +52,7 @@ export const createLocalStorageAdapter = ({
   key?: string
 } = {}): Adapter => {
   if (key) {
-    return createSharedStoreAdapter({
-      adapter: createLocalStorageAdapter(),
-      key,
-    })
+    return createSharedStoreAdapter(key)
   }
 
   return {
@@ -73,13 +64,7 @@ export const createLocalStorageAdapter = ({
       localStorage.removeItem(key)
     },
 
-    getItem: async (key) => {
-      const serializedValue = localStorage.getItem(key)
-
-      if (!serializedValue) return undefined
-
-      return JSON.parse(serializedValue)
-    },
+    getItem: async (key) => parseJson(localStorage.getItem(key)),
 
     setItem: async (key, value) => {
       localStorage.setItem(key, JSON.stringify(value))

src/lib/state/persistence/adapters/createLocalforageAdapter.test.ts

@@ -0,0 +1,13 @@
+import { describe, expect, it } from "vitest"
+import { createLocalforageAdapter } from "./createLocalforageAdapter"
+
+describe("createLocalforageAdapter", () => {
+  it("should read a value that is not JSON as nothing stored", async () => {
+    const adapter = createLocalforageAdapter({
+      getItem: async () => "dark",
+      setItem: async (_, value) => value,
+    })
+
+    expect(await adapter.getItem("theme")).toBe(undefined)
+  })
+})

src/lib/state/persistence/adapters/createLocalforageAdapter.ts

@@ -1,16 +1,11 @@
 import type { Adapter } from "./Adapter"
+import { parseJson } from "./parseJson"
 
 export const createLocalforageAdapter = (forage: {
   getItem: (key: string) => Promise<string | null>
   setItem: (key: string, value: string) => Promise<string>
 }): Adapter => ({
-  getItem: async (key: string) => {
-    const serializedValue = await forage.getItem(key)
-
-    if (!serializedValue) return undefined
-
-    return JSON.parse(serializedValue)
-  },
+  getItem: async (key: string) => parseJson(await forage.getItem(key)),
 
   setItem: async (key: string, value: unknown) => {
     await forage.setItem(key, JSON.stringify(value))

src/lib/state/persistence/adapters/parseJson.ts

@@ -0,0 +1,13 @@
+/**
+ * Parses a stored value. A value that is not JSON counts as nothing stored,
+ * so it does not block the hydration.
+ */
+export const parseJson = (serializedValue: string | null): unknown => {
+  if (!serializedValue) return undefined
+
+  try {
+    return JSON.parse(serializedValue)
+  } catch {
+    return undefined
+  }
+}