Reference issue for deleted prop in signal handler

[andrejcremoznik]

https://github.com/cferdinandi/reef/blob/c8f8cc37e3467a1bf72315f404d145f9fa15a625/src/components/signal.js#L26-L30

If the goal is to return the deleted value with the emitter, you'll need to create a copy of it before deletion.

        deleteProperty (obj, prop) {
            let value = structuredClone(obj[prop])
            delete obj[prop];
            emit(type, {prop, value, action: 'delete'});
            return true;
        }

[cferdinandi]

Good catch! All of my testing had been done with primatives as values, but this will absolutely not work when the property is an object (for mutation reasons you noted).

I'll get this done when I can, but if you wanted to submit a PR, that would probably move things along faster.

[TALlama]

This change breaks anything that holds a Proxy object, because they cannot be cloned. Would it make sense to catch the resulting DataCloneError and fall back to using obj[prop] here without the cloning?

[cferdinandi]

@TALlama if I'm following your correctly, the returned property is no longer a reactive proxy, which is what you would expect it to be, yes?

I actually think that's ok, personally. The intent (in my mind) is to let you know what changed rather than provide a property that could trigger another reactive render.

I'm more than open to being shown why that's a bad idea, though!

[TALlama]

No, the issue is broader than that: if obj[prop] returns a Proxy of any sort (reactive or otherwise) the new line throws an exception, and the entire reaction stops cold. This means you can't use proxies inside your data model, when proxies should ideally be transparent to the calling code. It's actually even broader than that, since there are whole other classes of objects that can't be cloned (DOM nodes, fragments, functions, regexes, and more), and now none of those can be included in the data model (some of those seem like bad ideas but I could see value in others).

I'm not depending on that value being emitted (because it didn't used to work) so I'd be okay if the code caught the exception and just sent out null when obj[prop] can't be cloned, but principle of least surprise means it should likely send out obj[prop] itself.

In either case, though, I'd expect that Reef should recover from the DataCloneError that the new line makes possible. I'll happily contribute a PR with the fix, if you want; I just need to know which way to resolve the issue.

[cferdinandi]

Thanks for the additional information, @TALlama ! I'll look into this further.