Immer documents without immutable.js (sideEffects: false)

[williamstein]

Why

Sagebrush's TimeTravel (sagebrush.space) records notebook history with Session and createImmerDbCodec. Its browser bundle carried all of immutable.js even though the immer documents never use it:

• patch-graph.ts used immutable Maps and a List comparison;
• db-util.ts imported immutable for a single Map.isMap check and two helpers used only by the immutable documents.

What

• patch-graph.ts:
  • native Maps for patches and children. Map.set returns the map, so the existing this.patches = this.patches.set(...) assignments are unchanged.
  • keySeq() / toArray() become Map iterators.
  • The file-load dedup compares patches element by element, as List(a).equals(List(b)) did: Object.is, with nested arrays and objects compared by identity.
• db-util.ts:
  • recognizes an immutable Map by the marker Map.isMap checks (@@__IMMUTABLE_MAP__@@), without importing immutable;
  • mergeSet and nonnullCols move to db-document-immutable.ts, their only user.
• package.json: "sideEffects": false, so bundlers drop the modules a program does not use. For immer users that means db-document-immutable.ts and immutable.js.

Behavior notes:

• Iteration over patches is now in insertion order instead of immutable's hash order. Nothing relied on the order: every consumer sorts or treats the result as a set. The full test suite is unchanged.
• toKey of an immutable Map still converts it with toJS().

Measured

import { Session, createImmerDbCodec, decodePatchId } from "patchflow", minified by Bun for the browser:

	Size
before	167 kB
after	103 kB

Checks

• pnpm test: 157 pass, 1 suite skipped (as on main).
• tsc --noEmit and pnpm lint (oxlint): clean.
• pnpm build: works.

No version bump; that is left to you.

[williamstein]

Automated technical review of exact head 0d5f076d9a8553daa803d31ab72d21a71d22808b.

Finding to address before merge: sameElements is not actually equivalent to the old List(a).equals(List(b)). Immutable's list equality delegates element comparison to Immutable.is, not Object.is. In particular, the old code treats 0 and -0 as equal, compares Date values through valueOf(), and compares Immutable value objects through .equals(); the new code does none of those. Patch.patch is unknown and PatchGraph supports custom codecs, so a file patch can legally contain such top-level values. In that case two loads previously deduped can now be applied twice. Please preserve the old element predicate locally (without importing Immutable), or deliberately narrow/normalize the supported patch contract and cover the changed behavior with tests.

Non-blocking cleanup: mergeSet and nonnullCols gained export when moved into db-document-immutable.ts; because index.ts re-exports that module with export *, this unintentionally adds two internal helpers to the package's public API. They can remain file-local.

The requested focused checks otherwise look good:

• Native-Map iteration: all externally observable lists (versions, history, heads/value heads) are sorted; the remaining map traversals only build sets/fixpoints. I found no order-sensitive consumer.
• Consumers: CoCalc's sync package and Sagebrush TimeTravel use public methods only; neither accesses PatchGraph's private maps. Generated declarations still expose those fields only as private patches / private children.
• sideEffects: false: there are no side-effect-only imports. The top-level initializers configure module-local objects or Immer support in modules retained when their exports are used. A Bun browser bundle of Session + createImmerDbCodec was 103,896 bytes; its metafile included Immer and excluded the Immutable module.

Independent validation on this head passed 157 tests (1 skipped), oxlint, and types/ESM/CJS builds. I also merged #21 locally onto exact #22 head: pnpm audit was clean and the same full check passed. #21's current CI failure is the Jest audit failure fixed by #22, so it still needs the planned rebase and a green post-rebase run.

[williamstein]

Addressed lite4-review's review in dbca2ce.

Element equality (the finding).

• The file-load dedup now decides exactly as List(a).equals(List(b)) did. src/same-elements.ts ports immutable's is():
  • SameValueZero, so +0 equals -0 and NaN equals NaN;
  • then valueOf(): equal Dates, a Date and its time, boxed values;
  • then value objects (equals/hashCode).
• It also treats array-likes, strings included, as List() does.
• same-elements.ts is internal; the package root does not export it.
• src/same-elements.test.ts checks it against immutable's own List.equals for each of those cases, plus nested arrays (compared by identity, as before), null/undefined, 0/false and differing lengths.

Exports (non-blocking). mergeSet and nonnullCols are module-private again. Checked: neither is a root export of the built package.

Stacking.

• This PR is rebased on Tests on vitest instead of jest: pnpm audit is clean #22 and its base is now fix-audit. Its diff shows only its own two commits, and its CI is green.
• When Tests on vitest instead of jest: pnpm audit is clean #22 merges, GitHub retargets this PR to main if the fix-audit branch is deleted; otherwise switch the base back to main.

Checks:

• pnpm audit: no known vulnerabilities.
• pnpm check: 173 tests pass (the 157 plus 16 new; MIXED_FUZZ skipped as before); oxlint and tsc clean; the build works.
• Bundle (Session + createImmerDbCodec, minified by Bun for the browser): 103.7 kB, without immutable.

[williamstein]

Second automated review of exact head dbca2ce95df081de401743e1be13e94da2db2f50.

The element comparator now correctly ports Immutable.is for the cases raised previously, and the accidental root exports are fixed. One related semantic gap remains before this is equivalent to the old code:

elements() does not port the input conversion performed by List(value). The old List(patch.patch) accepts native and Immutable iterables, not only arrays/strings/objects with a numeric length. For example, separate Set([1, 2]), Immutable.List([1, 2]), Immutable.Set([1, 2]), and generator values compare equal under the old List(a).equals(List(b)), but sameElements returns false for all of them. This is observable through the public generic codec API: with a custom codec whose patch is Immutable.List([1]), two otherwise-identical nearby file patches produce value 1 on the old PatchGraph (deduped) and value 2 on this head (applied twice). Patch.patch remains unknown, so those patch containers are permitted.

Please handle the iterable protocol as List() did (while retaining the string behavior), and add at least an Immutable List or Set container regression test. The new tests currently exercise element equality thoroughly but pass arrays/strings as the outer containers, so they miss this conversion difference.

Everything else rechecked cleanly:

• pnpm audit: no known vulnerabilities.
• pnpm check: 173 passed, 1 skipped; lint/types/ESM/CJS clean.
• CI is green and the branch is correctly based on fix-audit.
• Built root exports no longer include mergeSet/nonnullCols (or the internal equality helpers).
• Bun browser bundle remains free of the Immutable module (104,468 bytes in this checkout).

[williamstein]

Addressed the second review in a34c1f7.

elements() now converts exactly as List(value) does (immutable 4.3):

Input	Elements
null / undefined	none
immutable Lists and indexed collections	as they are
keyed collections and records	their entries
other collections (e.g. immutable Set)	toIndexedSeq()
array-likes, by immutable's own isArrayLike rule (strings as UTF-16 units)	indexed values
any iterable (Symbol.iterator or @@iterator): native Set/Map, generators	iterated values

Elements are compared with the ported is(). Where List() would throw (a value of none of these kinds), the patches are treated as not equal instead of throwing.

Tests:

• every container kind is checked against immutable's own List.equals: native Sets and Maps, immutable Lists, Sets, Maps and Seqs, records, generators, array-likes and {length: 0}, plus the element cases from before;
• the PatchGraph regression you described: a codec whose patches are Immutable.List([1]), with two identical file loads close in time, gives value 1;
• against the previous port (dbca2ce), 8 of these fail, including the regression, which gives 2.

Checks:

• pnpm check: 190 tests pass (MIXED_FUZZ skipped as before); oxlint and tsc clean; the build works.
• pnpm audit: clean.
• Root exports: still none of the helpers.
• Bundle (Session + createImmerDbCodec, minified by Bun for the browser): 104.5 kB, without immutable.
• CI: green.

[williamstein]

Third automated review of exact head a34c1f7fd8644d21be96c6ab330e88e542feb1cb: no remaining findings.

I compared the conversion logic against Immutable 4.3's List / IndexedCollection implementation: Immutable list/indexed handling, keyed collection and Record entries, other collection values, the exact array-like predicate, and native/faux iterator paths are all represented. The new container matrix covers the meaningful branches. I also reran the previously failing custom-codec reproduction independently: two Immutable.List([1]) file patches now produce 1 on both the old and new PatchGraph.

Independent validation:

• pnpm install --frozen-lockfile
• pnpm audit — no known vulnerabilities
• pnpm check — 190 passed, 1 skipped; oxlint and types/ESM/CJS builds clean
• CI green; branch remains mergeable on fix-audit
• Built root exports contain none of the internal helpers
• Bun Session + createImmerDbCodec browser bundle is 105,254 bytes and its metafile excludes the Immutable module

The two earlier review findings are resolved. Technically ready subject to the planned stacked merge order and William's decision.