Repository navigation
build: drop dot-prop-immutable, inline a local setIn with identical semantics - #81
Conversation
…emantics
dot-prop-immutable has been unmaintained since 2020 and the library only
ever used its `set` function (in updateDataFromEvents). Replace it with a
small internal module, src/lib/Form/setIn.js, whose behavior was validated
against dot-prop-immutable@2.1.0 using a differential test battery
(removed from the final diff along with the dependency).
Reproduced semantics, pinned by tests:
- dot paths with numeric indexes ('user.email', 'items.0.label'), `\.`
escapes, `$end` and `+`-prefixed indexes on existing arrays
- structural immutability: only the written branch gets new references,
untouched siblings keep their identity (React.memo contract)
- missing intermediates created as plain objects, even for numeric
segments ({} + 'list.1' => { list: { 1: 'x' } }, not an array)
- arrays only index-addressed when they already exist; non-integer
segment on an array throws, out-of-bounds indexes extend the array
- scalar intermediates spread-cloned (numbers/booleans => {}, strings =>
char-indexed objects); null intermediate with remaining segments
throws a TypeError, like the original
- function values act as updaters
One deliberate hardening deviation: property writes use
Object.defineProperty, so a '__proto__' segment creates a plain own
property instead of invoking the inherited setter — no path can mutate a
shared prototype.
Closes the last item of the audit issue #57 §3.
hugomallet
left a comment
There was a problem hiding this comment.
Tool-assisted self-review (Claude Code, review53 skill) — posted as a comment since author = reviewer.
Verdict: LGTM — no blocking findings
Correctness — setIn.js line by line
- Structural sharing is correct with no mutation leak: every traversed level is cloned exactly once (
slicefor arrays, spread otherwise), writes only ever target the fresh clone of the current frame, and recursion descends into the original child — siblings keep reference identity, nothing beyond the written path is cloned. Traced all branches; the immutability tests (setIn.test.js:99-132) pin both directions (no source mutation + sibling identity). - Independent differential check (not just trusting the PR's removed battery): I re-ran
setInagainst the realdot-prop-immutable@2.1.0on 46 fixed edge cases + 800 pseudo-random data/path combinations (nested arrays/objects/scalars/null/undefined,$end,+n,\., empty segments, updater fns, error messages). Fully convergent, including sparse-array holes (new Array(3)→ hole layout preserved identically by both). - All 151 lines earn their keep: ~100 are JSDoc documenting the reproduced semantics; the executable core is ~50 lines with no dead branch. The exotic handling (
$end,+n, non-integer-on-array throw, scalar/null intermediates) is exactly the dpi surface, not over-engineering.
Reachability of the exotic features → parity is the right call
handleFieldChange(event | string, value) is exposed through the form context (Form.js:238,406) and accepts an arbitrary string path; DOM name attributes are arbitrary strings too. So $end, +n, \., numeric-segment-on-absent-target and the array error message are all reachable from the public API by consumers today. Dropping any of them would be a silent behavior change smuggled into a dependency-removal PR — parity first, simplification (if ever) as its own semver-discussed change. Correct scoping.
Security
defineOwn(Object.defineProperty,setIn.js:90-97) neutralizes both vectors: a__proto__segment becomes a plain own data property (no inherited setter invoked), andconstructor.prototype.xonly ever writes into a fresh spread-clone ofObject.prototype(own enumerable props only →{}), never the shared prototype. The spread clone (vs dpi'sObject.assign, which uses[[Set]]) also protects re-cloning of objects that already carry an own__proto__key.- Both vectors are constrained by tests (
setIn.test.js:150-172), including result-prototype and own-descriptor assertions, plus the benignconstructor.xcase.
Packaging / docs
package.json+yarn.lock: only thedot-prop-immutableentry removed, nothing else touched. Correct.- CHANGELOG: under
[Unreleased] › Changed, accurately states behavior-unchanged + the single hardening deviation. Consumer-visible framing is right. - JSDoc in
helpers.jsupdated coherently ({@link setIn});updateDataFromEventssignature untouched, all call sites unaffected. - Separate module vs
helpers.js: separate file is the right call — matches the existing precedent ofthrottle.js(single-purpose inlined utility with its own test file in the same folder), and keeps the 300-linehelpers.jsfrom absorbing a self-contained algorithm.
Checks
153/153 tests pass locally; ESLint + tsc --noEmit clean on the changed files (repo config); CI green across build + @types/react 16-19 matrix.
Minor (optional, non-blocking)
setIn.test.js: the__proto__pollution test only exercises the segment at depth 0; a one-line nested variant (setIn({ user: {} }, 'user.__proto__.polluted', true)) would pin the guard at arbitrary depth. The mechanism is uniform so this is belt-and-suspenders.
Review follow-up on #81: the __proto__ hardening test only exercised the dangerous segment at depth 0. Add nested variants ('user.__proto__.polluted' and 'user.constructor.prototype.polluted') asserting that at any depth the write stays a plain own property, the nested clone's prototype is untouched and nothing leaks through the prototype chain. Mutation-checked: neutralizing the defineOwn guard makes the nested test fail (the inherited __proto__ setter rewrites the clone's prototype). An afterEach cleans Object.prototype.polluted as a safety net so a future regression cannot leak into other tests.
Part of the final batch of audit #57 (§3, last item): remove
dot-prop-immutable, unmaintained since 2020, from the published dependencies.What
The library only ever used
immutable.set(data, path, value)inupdateDataFromEvents(src/lib/Form/helpers.js). This PR replaces it with a small internal module,src/lib/Form/setIn.js(1.6 kB in dist, 0.72 kB gzip), and removes the dependency frompackage.json/yarn.lock.Semantics parity
The implementation was converged against the real
dot-prop-immutable@2.1.0using a temporary differential test battery (38 fixed cases + 500 deterministic pseudo-random paths over a mixed fixture, comparing results, JSON serialization, array-ness, thrown error types/messages, and sibling reference identity). It passed on the first run and was removed from the final diff together with the dependency, as it can no longer import the package.Verified & pinned by the permanent test suite (26 tests in
src/lib/Form/setIn.test.js):user.email,items.0.label),\.escapes,$endand+-prefixed indexes on existing arrays, non-integer segment on an array throwsArray index '…' has to be an integer, out-of-bounds indexes extend the arraytoBeon references, preserving theReact.memo/PureComponentcontract documented onupdateDataFromEventssetIn({}, 'list.1', 'x')→{ list: { 1: 'x' } }, not an array) — that is what dot-prop-immutable really does; arrays are only index-addressed when they already exist{}, strings → char-indexed objects);nullintermediate with remaining segments throws aTypeError, exactly like the originalOne deliberate deviation (hardening, documented in the module JSDoc): writes go through
Object.defineProperty, so a__proto__path segment creates a plain own property instead of triggering the inherited__proto__setter (which in the original rewrote the clone's prototype). Combined with writes only ever targeting fresh clones, no path can mutate a shared prototype (__proto__.x,constructor.prototype.xcovered by tests).Mutation proofs (tests bite)
Array.isArraybranch (treating arrays as objects) → all 6 array-index tests fail (includingArray.isArray(result.items)assertions).Verification
yarn lintclean (run with--no-eslintrc -c .eslintrc.jslocally to bypass a stray config in the temp parent dir; CI runs it normally),yarn type-checkclean, type matrix green on @types/react 16/17/18/19yarn distgreen;grep -ri dot-prop dist/→ zero referencesnpm publish --dry-rungreen (17 files, 19.4 kB,dist/Form/setIn.jsincluded)src/lib/Form/setIn.js+ test,src/lib/Form/helpers.js(import swap + JSDoc),package.json,yarn.lock,CHANGELOG.mdRefs #57