fix: Don't unfreeze/copy targets w/object getters.

[stephenh]

isFrozen treated any non-writeable descriptor as "needs unfreezing", when really it is only non-configurable descriptors that break the proxy get trap (specifically when the get trap returns a different value than the target would).

This means proxy-compare was unnecessarily unfreezing/copying targets that had object getters (non-writeable but they are configurable).

Granted, this behavior (an unnecessary copy) is I believe only observable in my local experiments where I'm using proxy-compare against non-snapshot instances, and also doing mutations through them, like:

const o = { count: 1, ...getter... }
const p = createProxy(o)
// doesn't mutate `o` b/c createProxy made an "unfrozen" copy
p.count = 2

But, the experimental nature of that aside, I believe this still is a minor-but-valid behavioral correction in proxy-compare.

[codesandbox-ci[bot]]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit cac10e40d0ab1f7c1d69e70004579d1606c02d62:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration
React	Configuration
React Typescript	Configuration

[dai-shi]

> o1 = { a: 1 }
{ a: 1 }
> Object.freeze(o1)
{ a: 1 }
> Object.getOwnPropertyDescriptors(o1)
{
  a: { value: 1, writable: false, enumerable: true, configurable: false }
}
> p1 = new Proxy(o1, { get() { return 0 } })
Proxy [ { a: 1 }, { get: [Function: get] } ]
> p1.a
Uncaught:
TypeError: 'get' on proxy: property 'a' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '1' but got '0')
> o2 = {}
{}
> Object.defineProperty(o2, 'a', { value: 1, writable: false, enumerable: true, configurable: false })
{ a: 1 }
> p2 = new Proxy(o2, { get() { return 0 } })
Proxy [ { a: 1 }, { get: [Function: get] } ]
> p2.a
Uncaught:
TypeError: 'get' on proxy: property 'a' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '1' but got '0')
> o3 = {}
{}
> Object.defineProperty(o3, 'a', { value: 1, writable: true, enumerable: true, configurable: false })
{ a: 1 }
> p3 = new Proxy(o3, { get() { return 0 } })
Proxy [ { a: 1 }, { get: [Function: get] } ]
> p3.a
0
> o4 = {}
{}
> Object.defineProperty(o4, 'a', { value: 1, writable: false, enumerable: true, configurable: true })
{ a: 1 }
> p4 = new Proxy(o4, { get() { return 0 } })
Proxy [ { a: 1 }, { get: [Function: get] } ]
> p4.a
0

So, do we need to copy only if writable or configurable is false?

[stephenh]

Hrm, yeah, to see what would happen, I completely commented out the copy, and the results were:

(All of the failures were the get trap breaking.)

So looks like properties must be both non-writeable and non-configurable to break the proxy trap.

I've pushed up a change + extra tests that reflect that.

Lmk if the hasFrozenishProperites name is too cute. :-) I started out typing out hasNonConfigurableAndNonWr... and went with something shorter instead.

Fwiw I think I just realized that, until your recent change to Valtio, Valtio was making snapshots (1 copy), freezing them (another internal copy?...not really sure), and then here in proxy-compare we would need to unfreeze (another copy) to let the proxy work anyway. So at least 2, maybe 3 copies.

So, given that realization, removing the Object.freeze from snapshots seems like a good call!

[dai-shi]

Thanks. I'll check the code and refactor for my preference later.

freezing them (another internal copy?...not really sure)

Object.freeze() mutates the object (= side effects). It shouldn't copy as it's fairly lightweight.

[stephenh]

Sounds good.

At first I thought more changes could be made, but after going in a circle a bit, in retrospect all I did was add a && !obj.configurable. :-) :shrug:

Happy with however you merge it.

[dai-shi]

@stephenh This looks great. Thanks for your work! Would you check my changes and adjust your source code comments?

[stephenh]

I looked through the changes and they look good, thanks @dai-shi !

I actually didn't think of pushing the change even further, i.e. to renaming IS_FROZEN -> IS_TARGET_COPIED_PROPERTY, but I like that.

Also yep pushed up an updated comment, and removed another inline comment as now unnecessary.

Thanks!