Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

window.document cannot be mocked / replaced #99

Open
buschtoens opened this issue Mar 26, 2019 · 4 comments · May be fixed by #101
Open

window.document cannot be mocked / replaced #99

buschtoens opened this issue Mar 26, 2019 · 4 comments · May be fixed by #101

Comments

@buschtoens
Copy link
Contributor

buschtoens commented Mar 26, 2019

I tried something like this:

  hooks.beforeEach(function() {
    class DocumentMock extends EventTarget {
      // ... some custom `window.document` mock
    }

    window.document = new DocumentMock();
  });

But this throws the following error:

TypeError: 'set' on proxy: trap returned truish for property 'document'
  which exists in the proxy target as a non-configurable and non-writable
  accessor property without a setter

The reason being

Take a look at the following, section 9.5.9 of the ECMA spec:

http://www.ecma-international.org/ecma-262/6.0/#sec-proxy-object-internal-methods-and-internal-slots-set-p-v-receiver

A riveting read I'm sure you'll agree.

I believe the two keys lines are:

Let booleanTrapResult be ToBoolean(Call(trap, handler, «target, P, V, Receiver»)).

and the equally esoteric:

If targetDesc is not undefined, then
a. If IsDataDescriptor(targetDesc) and targetDesc.[[Configurable]] is false and targetDesc.[[Writable]] is false, then

i. If SameValue(V, targetDesc.[[Value]]) is false, throw a TypeError exception.

There is this relevant comment in the NOTE section:

Cannot change the value of a property to be different from the value of the corresponding target object property if the corresponding target object property is a non-writable, non-configurable own data property.

StackOverflow

With the current setup, it is not possible to completely mock properties like document.

Can we maybe add a method / export a function to directly set properties on the holder? This way we could do something like:

import window, { mock } from 'ember-window-mock';

module('foo', function(hooks) {
  hooks.beforeEach(function() {
    class DocumentMock extends EventTarget {
      // ...
    }

    mock('document', new DocumentMock());
  });
});
@simonihmig
Copy link
Owner

Hm, that's weird, didn't know of that behaviour!

Wouldn't be opposed to it, if you want to contribute this!? 😉

The API of your example would only allow to mock a property on the root level of window, but we also wrap nested objects in Proxies to enable overriding readonly sub-properties (like window.navigator.userAgent). That wouldn't be possible in this case, unless you want to enable something like mock('foo.bar', ...), which would probably be difficult to implement. Don't know if there is a real-world use case though?

Alternatively maybe add some method to any Proxy we create, like window._mock('document', ...), as this would also allow window.foo._mock('bar', ...).

@buschtoens
Copy link
Contributor Author

Hm, that's weird, didn't know of that behaviour!

I also did not know before. 😅

Wouldn't be opposed to it, if you want to contribute this!? 😉

Sure, I'd love to!

The API of your example would only allow to mock a property on the root level of window, but we also wrap nested objects in Proxies to enable overriding readonly sub-properties.

Good point. I usually tend to avoid mocking things partially, but I guess especially for window and its sub-properties this does not really apply, so we should support a way to do this.

Alternatively maybe add some method to any Proxy we create

I thought about that too, but rejected it, because it would change the behavior of code that iterates over properties on objects. We also can't guarantee, that users / libraries might not already use the _mock key.

I'd either allow specifying paths, like you suggested, or allow the user to pass the target proxy as the first parameter:

mock(window.document, 'querySelector', (selector) => ...);

But I think I prefer:

mock('document.querySelector', (selector) => ...);

@buschtoens
Copy link
Contributor Author

buschtoens commented Mar 28, 2019

Another alternative would be a function to retrieve the holder for any proxy from a WeakMap:

import window, { getHolder } from 'ember-window-mock';

module('foo', function(hooks) {
  hooks.beforeEach(function() {
    class DocumentMock extends EventTarget {
      // ...
    }

    getHolder(window).document = new DocumentMock());
  });
});

Also tangentially related: I think it would reduce complexity, if we'd remove all the special handling here and just assign these properties to the holder:

https://github.com/kaliber5/ember-window-mock/blob/fa60f4119a36a39cbc8c7ce4fcdfea9249d5e913/addon-test-support/-private/window.js#L18-L25

This would then also allow users to mock objects like location through the holder.

@simonihmig
Copy link
Owner

Another alternative would be a function to retrieve the holder for any proxy from a WeakMap:

I think I would prefer the mock function. Exposing the holder directly leaks too much of the (Proxy-based) implementation into the API IMHO. Also having the mock() we would have more control, maybe later add some checks, so not everything can be mocked or whatever.

Also tangentially related: I think it would reduce complexity, if we'd remove all the special handling here and just assign these properties to the holder

Agree, good point!

I thought about that too, but rejected it, because it would change the behavior of code that iterates over properties on objects.

Haven't thought much about the implementation yet. But re: iterating, you could define it as not enumerable I think, or expose that function through the Proxy's get-handler maybe? Just thinking aloud, but I guess it should be doable to hide that function from enumeration!?

It would probably make having access to the holder easier, as you could closure over it (is that how you call it, I have no idea!? 😂). Otherwise you would probably have to apply that WeakMap approach you mentioned, or assign some private property (Symbol) to the Proxy, so mock() gets hold of the holder 🙃.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants