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

Consider using _.isObjectLike rather than _.isPlainObject within td.object #224

Closed
traviskaufman opened this issue Mar 28, 2017 · 6 comments

Comments

@traviskaufman
Copy link

traviskaufman commented Mar 28, 2017

Given this code:

const td = require('testdouble');

class MyClass {
  doSomething() {}
}

function callDoSomething(myClass) {
  myClass.doSomething();
}

const mock = td.object(new MyClass());
callDoSomething(mock);
td.verify(mock.doSomething());

When run on a browser that doesn't support proxies - such as IE11 - this code will throw an error saying:

Error: Error: testdouble.js - td.object - The current runtime does not have Proxy support, which is what testdouble.js depends on when a string name is passed to `td.object()`.

I believe this to be a false positive, with the offending code being

if (_.isPlainObject(nameOrType)) {

This code uses _.isPlainObject, which returns false for instances of objects whose [[Prototype]] is not Object.prototype. This is problematic for instances of classes, who's [[Prototype]] is the constructor.

I'm wondering if you'd be willing to switch to _.isObjectLike instead of isPlainObject? This will return false for functions / constructors, but true for instances.

I'm able to work around the _.isPlainObject limitation by creating a fake constructor using td.constructor, and then instantiating that instead, but it is a bit more boilerplate.

Thanks!

@searls
Copy link
Member

searls commented Mar 29, 2017

I think you are probably right and that _.isObjectLike is closer to what we desire here.

However, I'd like to understand why td.constructor() isn't a better fit for your needs in this case, given that your fake is an instantiated Something.

@searls
Copy link
Member

searls commented Mar 29, 2017

I dunno. I guess I could go either way after writing this out:

const mock = td.constructor(MyClass);
callDoSomething(new mock());
td.verify(mock.prototype.doSomething());

@searls
Copy link
Member

searls commented Mar 29, 2017

Hmm, there are really three affected areas by this distinction and I think it warrants some thinking before we change anything.

  • td.matcher.contains - would have to put regex above and then ask ourselves if there are cases this could screw up
  • td.replace, but this seems like it'd be safe since the only other path would be an error
  • td.object as you described, proxy is only for string, so i think that makes sense there, but it'd have to be demoted below array.

@searls
Copy link
Member

searls commented Mar 29, 2017

hmm, this isn't quite an obvious fix. If we just swap isObjectLike, the td.object() call still won't work, because such an object will not give up its functions via _.functions so none of its properties will be replaced with doubles.

Probably need to spend a little more time looking at this…

@searls
Copy link
Member

searls commented Mar 29, 2017

I've had the night to reflect on this and I think I want to recommend using td.constructor for any fake custom types, because they'll raise all sorts of issues (beyond the _.functions thing from last night), like getters/setters, etc.

Instead, I'm going to suggest we "fix" this by improving the message.

@searls searls closed this as completed in 1ca7fc1 Mar 29, 2017
searls added a commit that referenced this issue Jun 18, 2017
Okay, #224 should be complete with this change:

* Get rid of one-off prototypal function name finder (in favor of the
single gatherProps + filterFunctions functions)
* There was a bug in the replace-test that was somehow erroneously
passing in phantom before but this irritated it somehow (phantom 2.1.1
doesn't even support the class keyword), so I had to add a flag to
ignore the test under phantom—still passed under Chrome FWIW.
@searls searls reopened this Jun 18, 2017
@searls
Copy link
Member

searls commented Mar 25, 2018

Closing in favor of the approach taken in #264

@searls searls closed this as completed Mar 25, 2018
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

No branches or pull requests

2 participants