refactor: DRY up and support descriptors

[buschtoens]

As a first step towards #99, this PR removes code duplication and special handling.

I also found that Sinon uses property descriptors to stub methods in the following segment:

https://github.com/kaliber5/ember-window-mock/blob/fa60f4119a36a39cbc8c7ce4fcdfea9249d5e913/tests/unit/window-mock-test.js#L156-L179

Property descriptors were not supported by the previous proxy implementation, so I've added that as well. It only worked because the proxy stubbornly kept returning the noop, which made Sinon fallback to a simple assignment. But since alert et al aren't special handled any more, they are deleted by Sinon and the fallback doesn't work any more.

I also removed the _reset method. Instead you can pass the proxy to reset, like so:

reset(proxy.location);

Calling reset() without any arguments is equivalent to calling

reset(window);

When a proxy is reset, all properties and descriptors are discarded, including the ad-hoc proxies.

There's now also a getProxyConfig function that we can use to implement something like mock / getHolder.

[simonihmig]

Really love the direction where this is going. Much cleaner! Thanks a bunch for working on this!

Some questions though...

[simonihmig]

What's the reason for that assertion? Wouldn't it be possible to "override" a non-configurable property with our Proxy? As we also allow setting a value on a readonly property, so basically trying to allow fiddling with window as much as possible.

[buschtoens]

Unfortunately, no. 😞

Try running this:

// `window` being the original window
const descriptor = Object.getOwnPropertyDescriptor(window, 'document');
console.log(descriptor);
Object.defineProperty(window, 'document', descriptor); // works cause nothing changed
Object.defineProperty(window, 'document', { ...descriptor, get: () => 'foo' }); // breaks

Uncaught TypeError: Cannot redefine property: document

Because users would not be able to use Object.defineProperty for document in the real world, we must prohibit it here as well. If we were to allow and return true, the change would take effect, because the descriptor would have been added to the descriptors map, but a TypeError would be thrown nevertheless, because of these Invariants.

If users want to override properties like document, they need to use mock from #101. One limitation of mock is that is only allows you to set properties, but not descriptors. I think that's okay for the most part, but we could also export two functions, like set and defineProperty or something similar.

[simonihmig]

Because users would not be able to use Object.defineProperty for document in the real world, we must prohibit it here as well.

Not sure why?

a TypeError would be thrown nevertheless, because of these Invariants.

which invariant says that you cannot shadow a non-configurable property with another (configurable) one?

If users want to override properties like document, they need to use mock from #101.

Wasn't this the case where document was non-configurable and non-writable, which you assert with assertWritableDescriptor()?

I might totally miss something here though...

[buschtoens]

Because users would not be able to use Object.defineProperty for document in the real world, we must prohibit it here as well.

Not sure why?

Turns out, you can still modify a non-configurable descriptor. 😂
You can flip the writable flag for instance, but only to false and not back again. But you can't switch from value to get / set. It's probably safest, if we copy the descriptor from target over to holder and then try mutating it. If this throws an error, we know it's not allowed.

const obj = {};

Object.defineProperty(obj, 'foo', { configurable: false, writable: true, value: 'bar' });

// breaks
Object.defineProperty(obj, 'foo', { configurable: false, get: () => 'baz', set: undefined });

// works
Object.defineProperty(obj, 'foo', { configurable: false, writable: false, value: 'quux' });

// now also breaks
Object.defineProperty(obj, 'foo', { configurable: false, writable: false, value: 'quax' });

[buschtoens]

Ah, but this would then "poison" the descriptor on the holder as well.

[buschtoens]

It's now checked using a dummy object.

[simonihmig]

First, sorry for letting this wait for so long! Was a tough week...

I am still not really understanding why we cannot just define the property on the holder? In what case would that throw an error?

Your examples above are referring to defining a property an a regular object, that already has a descriptor. But do those examples apply also when we define the property on a holder of a Proxy, instead of the target itself?

[simonihmig]

This is exactly the same code as 4 lines below, so what purpose does that have? And how does that help when it would throw a TypeError, as we don't catch anything?

[buschtoens]

Right! It should be dummy instead of config.holder.

What this is supposed to do is check whether the original descriptor of the target is actually overridable. If it is not overridable, this line will throw an error. I did not explicitly catch that error as throwing an error in or returning false from Proxy#defineProperty will lead to the same result basically. However, to make it more explicit and to not rely on behavior which is not explicitly defined, I'll wrap this in a try / catch and return false.

The reason that we do this in the first place is that defineProperty will be executed, even if one of the invariant scenarios is hit. It has to return false though. If it does not, a TypeError will be raised.

L71 Object.defineProperty(config.holder, key, descriptor); would have been executed nevertheless though, which means that the defineProperty did succeed anyway. I will add some tests around this.

[buschtoens]

Hm, while writing the tests, I noticed some more inconsistencies. I guess I'll have to manually codify all invariants and some more rules. 🙈

[simonihmig]

@buschtoens could you also please add some tests covering the new support for descriptors? Especially for eventual edge cases (overriding non-writable and/or non-configurable props), where I still struggle to get a grasp of (sorry about that! 😉), see the other comments.