`copyTargetObject` seems not work as expexted when the target is an frozen array object with symbol properties.

[vikiboss]

The key source code in this package is as follows, it use Array.from to copy array shallowly when the array is frozen.

https://github.com/dai-shi/proxy-compare/blob/6aeec50ed3e807f2a8a089d316502bcb4d0e966a/src/index.ts#L47-L64

When the obj is an array object, Array.from & [... obj] will remove all the symbol properties, which causes the following code not to work properly:

import { createProxy } from 'proxy-compare'

const symbol = Symbol('test')
const arr = [1, 2, 3, 4]
arr[symbol] = () => arr // define a symbol property in array object
const obj = { arr }

// deep freeze
Object.freeze(obj)
Object.freeze(obj.arr)

const proxy = createProxy(obj, new WeakMap())

// undefined, can't access symbol property
console.log('proxy.arr[symbol]:', proxy.arr[symbol]?.())

But a frozen plain obj with symbol properties (not an array) works as expected.

So, is it expected? Is there any workaround to solve it? Or might we need to implement a specific method to copy array properly?

Thank you for taking the time to consider this issue.

btw, I like this package so much, thanks your excellent work on it.

[dai-shi]

btw, I like this package so much, thanks your excellent work on it.

thanks!

Array.from & [... obj] will remove all the symbol properties,

Hmm, yeah, it expects Array to be only have number index. I think If you want symbol (or string) properties, you want to prefer using non-array object.

That said, if we have an idea for a reasonably simple implementation, we can consider supporting it. What would be your idea?

I'm curious, does it work well if an array isn't frozen?

[vikiboss]

I'm curious, does it work well if an array isn't frozen?

Yep, it does work, but we don't want the array to be modified by accident in some read-only scenarios. It works but may not work perfectly if not frozen.

[vikiboss]

I'm not very clear about the comment above the Array.from: // Arrays need a special way to copy, is there any cases? I find it works well when no Array.isArray judgement.

[dai-shi]

It means, arrays can't be copied like objects.

Yep, it does work

Oh, yeah, if it's not frozen, we don't need to copy it anyway. Right.

[dai-shi]

I find it works well when no Array.isArray judgement.

Oh, really? But the resulting object is not an array, right? Check it with Array.isArray.

[vikiboss]

But the resulting object is not an array, right?

You are exactly right, I missed it.

I think I might need to rethink my work. Thanks for your patience!

[dai-shi]

Just a note to myself:

> a=[1,2,3]
[ 1, 2, 3 ]
> a.p = 4
4
> a
[ 1, 2, 3, p: 4 ]
> Object.getOwnPropertyDescriptors(a);
{
  '0': { value: 1, writable: true, enumerable: true, configurable: true },
  '1': { value: 2, writable: true, enumerable: true, configurable: true },
  '2': { value: 3, writable: true, enumerable: true, configurable: true },
  length: { value: 3, writable: true, enumerable: false, configurable: false },
  p: { value: 4, writable: true, enumerable: true, configurable: true }
}

getOwnPropertyDescriptors doesn't tell if a property is an index or not. We can copy all properties after Array.from, but that worsen the performance. We should be super careful about the performance.

[vikiboss]

The last question, is it possible to add a customCopyTargetObject option or something like that in createProxy?

I understand that you might want to keep this package simple and pure, but this advanced option will be helpful to handle certain scenarios.

Thanks for your patience again.

[ityuany]

I really like proxy-compare, we have relied on it deeply in our daily work.

I think using Array.from directly for handling arrays in copyTargetObject is not user-friendly because it will break the original behavior of the array.

proxy-compare should promise not to break the behavior of the original object or array, but only add some additional functionality on top of it.

In some scenarios, I may need to add some additional methods or properties to the Array for tagging purposes. This belongs to the user's own behavior and should not be disrupted just because proxy-compare is used.

These are some of my thoughts, hoping to provoke some discussion and reflection.

[dai-shi]

The last question, is it possible to add a customCopyTargetObject option or something like that in createProxy?

I understand that you might want to keep this package simple and pure, but this advanced option will be helpful to handle certain scenarios.

Thanks for your patience again.

Yeah, I want to make it more configurable, without complexity and overheads. I will work on #58 eventually.

[dai-shi]

I really like proxy-compare, we have relied on it deeply in our daily work.

Thanks!

proxy-compare should promise not to break the behavior of the original object or array, but only add some additional functionality on top of it.

I agree with the basic idea.

I think using Array.from directly for handling arrays in copyTargetObject is not user-friendly because it will break the original behavior of the array.

What can you do instead of Array.from?

[ityuany]

I think for arrays, we should use getOwnPropertyDescriptors after Array.from to remount all own properties. The reasons are as follows:

• The primary concern of proxy-compare should be functionality, and performance should only be considered on top of that.

• In real scenarios, only a small number of users will mount a large number of other properties on the Array, so it will not cause serious performance overhead.

At the same time, since we have deep dependencies on proxy-compare, this issue has caused some online failures for us. I don't know if you can provide us with an additional Canary Tag to help us temporarily fix the online failures.

Thank you very much for your help.

[dai-shi]

The primary concern of proxy-compare should be functionality, and performance should only be considered on top of that.

We had serious performance issue in Valtio, so I'm a bit nervous.

In real scenarios, only a small number of users will mount a large number of other properties on the Array, so it will not cause serious performance overhead.

But, what if an array is very very big? How can we know "other" properties?

I don't know if you can provide us with an additional Canary Tag to help us temporarily fix the online failures.

Do you mean you can override the deep dependency version? (If so, codesandbox build can help too.)

[ityuany]

But, what if an array is very very big? How can we know "other" properties?

I seem to understand your concerns. My understanding is that I only used getOwnPropertyDescriptors on the array itself, not looping through each element of the array. So there shouldn't be any serious performance issues. The properties obtained from getOwnPropertyDescriptors should be non-prototype linked, right?

Do you mean you can override the deep dependency version? (If so, codesandbox build can help too.)

If my plan is feasible, it will not cause serious consequences.

I hope you can help us release a version similar to 2.5.1-canary-[hash], and its npm tag can be "canary" instead of "latest". This way, it will not affect other users, and we can directly solve the problem by specifying the installation of 2.5.1-canary-[hash].

[dai-shi]

Well, my understanding is that if the array is very big, as we need to loop it twice, the performance can be bad.

You can open a draft PR, and it will create a codesandbox build. You should be able to specify the URL instead of 2.5.1-canary-[hash] for your installation.

[ityuany]

Well, my understanding is that if the array is very big, as we need to loop it twice, the performance can be bad.

Your concerns are indeed valid. In fact, we do not capture all attributes. We only need to capture symbols because our tagged attributes are all using symbols.

You can open a draft PR, and it will create a codesandbox build. You should be able to specify the URL instead of 2.5.1-canary-[hash] for your installation.

Understood, but due to the limitations of our network environment, we are unable to access the specified URL when installing dependencies on CI. We can only synchronize npm's lib and install it in our private repository. Therefore, we hope to have a canary version. Of course, if these helps cannot be provided, our final solution may be to fork 'proxy-compare'.

[ityuany]

In fact, we have drawn inspiration from your Valtio and implemented a feature.

  const snap = store.useSnapshot();

  const address = original(snap.user.address);

  useEffect(() => {
    console.log(address);
  }, [address]);

Our implementation solution is to attach a function named Symbol("original") to all snapshots. The purpose of the original function is to call snapshot[Symbol("original")]() on the snapshot. This works fine on pure objects, but it causes issues when working with arrays.

[dai-shi]

We only need to capture symbols because our tagged attributes are all using symbols.

If we consider this as your issue, as opposed to a general proxy-compare issue, I think you can copy your object in advance to passing it to proxy-compare. It's suboptimal and we hope to improve it in the future, but it might be a viable solution for now.

[dai-shi]

The purpose of the original function is to call snapshotSymbol("original") on the snapshot.

Another solution can be using WeakMap instead of Symbol to keep the original object.

[ityuany]

Feel the help you provided.

Let me rethink whether there are better ways to implement our features.