just reusing a new shared lib function

removing the casting... in favor of an if condition, this also help with the case were one key is not in the original target because it is the magic key

reusing shared lib func

this change is needed to accommodate for the new fact that sometimes the key is not installed on the original target, but on the shadow for the magic key

configurable, non-writable is the optimal definition since it can still change if needed, and the object invariants will not apply

When would we reuse the same shadow target and reset the property?

we will never reuse a shadow target, is that what you're asking?

Yes. I don't understand why the magicPropertyKey is set to configurable.

removing tons of stuff that were not used anywhere

this method follows @jdalton suggestion to avoid the creation of an extra array when doing concat. it also adds the magic key when present.

Could add ArrayReduce

unless this code allows someone destroying a property during the loop code, all the own keys will have a defined descriptor, right?

that's the thing, getOwnKeysAndMagicProperty will add a the magical key to the list... which might or might not be in the originalTarget.

all these libraries are using isUndefined helper for two reasons:

• it is supposed to be faster because it doesn't have to cast the value.
• It is easier to read and reasoning about what the condition is about.

same here for !isUndefined

these !isUndefined checks are weird. For the property key I'd suggest creating a method for: isPropertyKey following the semantics of ES:

1. If Type(argument) is String, return true.
2. If Type(argument) is Symbol, return true.
3. Return false.

the isUndefined is weird, yes... ideally we can have isDefined but I don't think TS allows to do a good declaration of that, @pmdartus might know how... but in general, we use isUndefined everywhere... see my previous message.

In this case, it is because the magical property key, which is well defined in the type system, could be undefined or a valid property key.

Here is how you would implement isDefined with typescript:

function isDefined<T>(value: T | undefined): value is T {
    return value !== undefined;
}

This is missing tests in the shared.spec.ts file.

I don't understand the comment above and why the key check has on the right-hand side of the expression. Performance wire moving the identity check on the left-hand side is preferable.

Here is how you would implement isDefined with typescript:

function isDefined<T>(value: T | undefined): value is T {
    return value !== undefined;
}

When would we reuse the same shadow target and reset the property?

that will be a type error because the argument is expect. we can make it optional though.

opinion: This is a bit complex to comprehend.

1. The proxy does not allow a magic property to be defined on the original target.
2. But it can be defined on the shadow target.

We are juggling quite a big to allow the magic property key to

1. be an indicator of a proxy object
2. be a defined property on the shadow target
3. support the original target to have a property key same as magic property key

We would greatly simplify if the magic property had only one purpose, to identify a proxy.

Add test cases for

1. Defining a new property using Object.defineProperty on the wet object
2. Delete the magic property key on the wet object
3. Have magic property defined on dry object, define same property on wet object, delete the property on wet object. Recheck if property exists and the value.

On top of this list, I would also add test to check the returned property descriptor of the magic propety on the wet object: { value: undefined, enumerable: false, configurable: false, writable: false }

@pmdartus this is the test that you asked for, in fact surface a problem, thanks:
b2aec8c

@ravijayaramappa I don't understand your test... can you elaborate?

Assuming that the magic property is discoverable, i was suggesting to add test cases for the corner cases.

const o = {};
const target = new ReactiveMembrane({
    tagPropertyKey: '$$MagicKey$$',
});

const wet = target.getProxy(o);
Object.defineProperty(wet, '$$MagicKey$$', {
    value: 'bar',
    configurable: true
});

#1
expect('$$MagicKey$$' in wet).toBe(true);
expect(object.keys(wet)).toEqual(['$$MagicKey$$']);

#2
delete wet["$$MagicKey$$"]
expect('$$MagicKey$$' in wet).toBe(false); // what should happen here?
expect(object.keys(wet)).toEqual([]); 

#3
const dry = {};
const target = new ReactiveMembrane({
    tagPropertyKey: '$$MagicKey$$',
});

const wet = target.getProxy(dry);
Object.defineProperty(dry, '$$MagicKey$$', {
    value: 'bar',
    configurable: true
});

Object.defineProperty(wet, '$$MagicKey$$', {
    value: 'baz',
    configurable: true
});

expect('$$MagicKey$$' in wet).toBe(true);
expect(object.keys(wet)).toEqual(['$$MagicKey$$']);
expect(wet['$$MagicKey$$]).toBe('baz');
expect(dry['$$MagicKey$$]).toBe('baz'); // value set on wet propagated to dry

delete wet["$$MagicKey$$"]
expect('$$MagicKey$$' in wet).toBe(false);
expect(object.keys(wet)).toEqual([]);
expect(dry['$$MagicKey$$]).toBeUndefined();

expect('$$MagicKey$$' in dry).toBe(false);
expect(object.keys(dry)).toEqual([]);
expect(dry['$$MagicKey$$]).toBeUndefined();

#4 would be the same test setup with the descriptor marked as configurable: false

this was only used by membrane, moving it there.

moved from base since it is only used here

magic key -> tag key property

It is safe to return the desc at this point. This one is pretty harmless and does not need to be wrapped.
Also, it is un-necessary to execute the if block in L147-150 on this desc.