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

Enable demand(function(){.....}).throw() scenarios when ctx is necessary #5

Closed
wants to merge 1 commit into from

Conversation

koresar
Copy link

@koresar koresar commented Dec 10, 2013

Examine the following scenario:

function MyClass() {
  this.variable = 'not initialized';
  function myFunction() {
    if (this.variable === 'not initialized') {
      throw new Error('Tried using uninitialized MyClass');
    }
  }
}

var myObject = new MyClass();
myObject.myFunction.must.throw(Error, /uninitialized/);

Clearly, the exception containing 'uninitialized' must be thrown, but instead I get different exception saying "null doesn't have 'variable' property". That is because this.actual.call(null) uses null as the context.

By passing this.actual as the context the fix enables the following testing syntax:

var myObject = new MyClass();
demand(function() { myObject.myFunction() }).must.throw(Error, /uninitialized/);

@moll
Copy link
Owner

moll commented Dec 10, 2013

Hey!

Thank you for caring! Fortunately or unfortunately this is a property of JavaScript that when you take the function off of an object myObject.myFunction it won't be tied to myObject any longer. Function context is in this context only a by-product of calling a function on an object and not inherent in the function.

this.actual will be the function itself. Passing itself as the context to itself shouldn't do much. :)

Anyways, I'm guessing what should solve your attempt is using Function.prototype.bind:

myObject.myFunction.bind(myObject).must.throw(...)

I've also now added a small explanation to Must.prototype.throw's documentation. Do you think the phrasing I used would help others in the future?

Let me know if that works for you!

@koresar
Copy link
Author

koresar commented Dec 10, 2013

Passing itself as the context to itself shouldn't do much. :)

My findings are different. The pull request works great for me using the syntax I showed. Surprisingly the this.actual context is the one the original function belongs to.

Phrasing is much better in this case demand(function (){myObject.myFunction()}).throw(...) than in this case myObject.myFunction.bind(myObject).must.throw(...)

Please also consider, that following would work as well:

demand( function() { myFactory(myClass).mixin({foo: 'bar'}).myFunction() }).throw(...);

Whereas with the syntax you have suggested I would need to have two lines of code and additional variable:

var myObject = myFactory(myClass).mixin({foo: 'bar'});
myObject.myFunction.bind(myObject).must.throw(...);

Test the change. :) What would be your findings?

@moll
Copy link
Owner

moll commented Dec 10, 2013

Oh, but you definitely should be able to use the demand style now, too, as the context of the surrounding function (I named it wrap below) shouldn't have any effect on what goes inside it:

demand(function wrap() { myFactory(myClass).mixin({foo: 'bar'}).myFunction() }).to.throw(...)

Or am I still misunderstanding something?

@koresar
Copy link
Author

koresar commented Dec 10, 2013

Ha! Silly me. Sorry. :)
Disregard this pull request, please.

@moll
Copy link
Owner

moll commented Dec 10, 2013

No prob. Happens to the best of us. ;-)

@moll moll closed this Dec 10, 2013
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 this pull request may close these issues.

2 participants