Snapshots can be mutated by adding or removing properties

[pastelmind]

Summary

Adding or removing properties from a snapshot object or array does not throw, even in strict mode.

'use strict' // Must be enabled in browser scripts / Node CommonJS

const state = proxy([5, 10, 15])
const snap = snapshot(state)

// snap[0] = 100 // Throws
snap[10] = 100 // Does not throw
snap.push(100) // Does not throw
snap.pop() // Does not throw

const state2 = proxy({ foo: 1 })
const snap2 = snapshot(state2)

// snap2.foo = 100 // Throws
snap2.bar = 2 // Does not throw

Although it appears that such invalid operations do not affect the proxy store itself, silently swallowing allowing these errors is undesirable, since the snapshot (correctly) errors on other types of mutations.

Edit: The snapshot can be mutated:

const p = proxy({ foo: 1 });
const s = snapshot(p);
console.log(s); // { foo: 1 }
s.bar = 2;
console.log(s); // { foo: 1, bar: 2 }
delete s.foo;
console.log(s); // { bar: 2 }

This seems to be a correctness issue.

Link to reproduction

https://codesandbox.io/s/sad-gauss-w364gf

Check List

Please do not ask questions in issues.

• I've already opened a discussion before opening this issue, or already discussed in other media.

Please include a minimal reproduction.

• I've added a link to a typescript playground or codesandbox with a minimal reproduction.

[pastelmind]

Preventing the addition of new properties should be trivial. We can call Object.preventExtensions() on the snapshot object here, just before returning it here:

diff --git a/src/vanilla.ts b/src/vanilla.ts
index 7844994..1e169a0 100644
--- a/src/vanilla.ts
+++ b/src/vanilla.ts
@@ -146,6 +146,7 @@ const buildProxyFunction = (
       }
       Object.defineProperty(snap, key, desc)
     })
+    Object.preventExtensions(snap)
     return snap
   },

I tested this locally and found no breaking tests. I can submit a PR if you want.

However, preventing deletion would be a bit more involved. It's hard to grok the details, but I believe snapshot properties were made configurable in ba53bf6 (#664) due to performance issues. Would it be too difficult to revert some of the changes?

If we want to avoid reverting the above, I suppose we might be able to wrap the snapshot object in its own Proxy with a deleteProperty handler that throws a TypeError. This sounds too complicated and risky, though.

[dai-shi]

We used to deep-freeze snapshot object. But, there's performance downside, because we had to deep-copy the object in proxy-compare. So, it means we took performance over correctness.

Object.preventExtensions() sounds good. If it doesn't break existing tests and it doesn't drop performance (but, we don't have tests for it), we can merge it.