addMethod returns a new assertion with flags copied over instead of this

[lucasfcosta]

This aims to solve #562.

Special thanks to @keithamus for the whole idea here, I just made some minor adjustments.

This PR should include:

• addMethod returning a new Assertion with flags copied over
• Tests for it
• Fix this - EDIT: Unfortunately there is one case we won't be able to cover, so this is only partially resolved. Please read this.

I'm currently writing the tests and I'm not sure if they're complete enough.
I was thinking about testing chaining some assertions as described on #562, testing the type of their return and if methods are really returning a new Assertion with the flags which should have been copied. Is this enough? If not, what else should be verified?

EDIT: This needs a review, please see this. I'll take a further look at it.

[lucasfcosta]

Here are the tests I thought were right for this.
If you guys think of more things that should be covered by this test, please share.

[keithamus]

Could you please add some assertions that feature the an keyword, and also feature the length keyword, similar to the original issue.

expect([1, 2, 3]).to.be.an.instanceOf(Array).and.to.have.length.below(1000)

[lucasfcosta]

Well thought @keithamus.
This fix appears to work with the expression you posted, but I went to the issue to test other examples and this appears to fail:

expect(my_object).to.have.a.deep.property("foo.bar").that.is.an("array").with.a.length.of.at.least(1);

The reason is the same you explained on the issue, we are getting back an assert function instead of an Assertion object.

I'll take a further look at this to find the exact reason.

Thanks again for your feedback, it was really useful 😃

[keithamus]

Hey @lucasfcosta thanks for doing that, and also awesome work adding some more tests and discovering another issue 😄

It looks like in that case, the .a. part turns the Assertion into a function, from which it doesn't recover. To wit:

> chai.expect({foo: [1,2,3]}).to
Assertion {
  __flags:
   { ssfi: [Function: addProperty],
     object: { foo: [Object] },
     message: undefined } }
> chai.expect({foo: [1,2,3]}).to.have
Assertion {
  __flags:
   { ssfi: [Function: addProperty],
     object: { foo: [Object] },
     message: undefined } }
> chai.expect({foo: [1,2,3]}).to.have.a
[Function: assert]
> chai.expect({foo: [1,2,3]}).to.have.a.deep
[Function: assert]

Luckily, it looks like the fix will be pretty much the same thing. L44 of addProperty has the same ternary.

[lucasfcosta]

EDIT: Please ignore this comment and just read this.

---

Ah, that makes sense.
Thanks for the insight here.

I was also doing some research on this too, shouldn't we create another assertion and transfer flags to it instead of returning this?

I'm not sure this is needed, I will do some further experiments with this, but it may be good to prevent further errors related to this.

[lucasfcosta]

Well, I've done some further studies on this and I've discovered that the explanation for why this (expect(my_object).to.have.a.deep.property("foo.bar").that.is.an("array").with.a.length.of.at.least(1)) is not working is the following:

We've got this line into our addChainableMethod.js file which returns a function as the getter for the property being added, therefore, when calling .length right after using the a getter we get back a function (the function which executes the chainingBehavior, as you can see here, so it has got its own .length property, instead of the .length property of the assertion.

What I've done on this PR partially solves our problem because when we return a new Assertion instead of this, it means we're returning an object which is not a function and therefore does not have the length property, that is why an.<something_here_will_return_new_Assertion>.length works (because we are "transforming" the assert function into another object which is not a function) and a.length does not (because a immediately returns a function, which has a length property).

---

How we could solve this:

This would be a very, very easy fix if we could just do: delete length before returning assert here. This would cause the .length property to be called from the prototype, which means it will call the assertion instead of the function's length property.

Unfortunately this is not available on all browsers because the .length property is configurable only on Firefox(Gecko) >= 37 and on Node.js.

Being able to delete .length would enable us to solve this with a single line of code, unfortunately this is not possible on older environments (or even on PhantomJS 2.1).

Currently we can only solve this for some cases, like the ones described above, in which we return a new Assertion when using addMethod or addProperty.

---

For Now

For now, I will just do the changes we've talked about on addMethod and addProperty, if we want to partially solve the problem, this is all we can do, the only case which will still not work is calling .length right after .a. If you have any other ideas please share.

On the current commits I improved the tests for both addMethod and addProperty and I have also simplified the way we're adding common language chains now that we are return a new Assertion on addProperty.

---

I hope I was clear, but if I wasn't don't be afraid to ask.

[keithamus]

Hey @lucasfcosta thanks for the awesome deep dive! Here's my commentary on this:

Unfortunately this is not available on all browsers because the .length property is configurable only on Firefox(Gecko) >= 37 and on Node.js.

This is because Function.length (and Function.name) are non-configurable as per ES5 spec (ES5$15.3.3.1 & ES5$15.3.3.2) but in ES6 it was decided that they should be configurable ($19.2.4.1 & $19.2.4.1). Being configurable also means they are delete-able ($9.1.10 if you're curious).

According to Kangax (here is the name is configurable test and here is the length is configurable test), Edge, Firefox, Chrome and Node (4 & 5) support this, but older browsers (IE<=11, older Android versions not using Chrome, all Safaris including iOS) do not.

For now....

Older IEs and Safari are basically immovable at this point. Its disappointing but it is what it is. Fixing this by ensuring we don't end up returning functions in the chain, as you've done in this PR - is useful of its own merit.

If we looked at deleting the properties (length and name) from the functions to help with newer browsers, we open ourselves up to issues from older browsers failing on these assertions. Consistent behaviour (even if it is consistently broken) is probably preferable over inconsistent behaviour - especially in a test framework. It should be our top priority that users should never have to debug Chai - they're already too busy debugging their code!

[lucasfcosta]

@keithamus thanks for the info on the Function.length and Function.name props! All the links to the spec were very useful. Awesome job!

I totally agree with you when you say that consistent behaviour is preferable, that's why this PR has only non-breaking and consistent changes, but, as I said, there's no way to solve chaining .length right after .a without problems on older browsers, so I kept it out of these commits.

This partially solves the problem, but it's still (in my opinion) better than nothing. What do you think about it?

Thanks for your feedback and for your help, without you this PR wouldn't have been possible 😄

[keithamus]

LGTM 😄

[Turbo87]

I'm getting ReferenceError: chai is not defined thrown here

[Turbo87]

I guess this problem has been shadowed by global.chai = require('../..'); in all tests...

[keithamus]

Good catch, thanks @Turbo87. Fancy making a PR for this?

[keithamus]

Awesome, I see you already have 🎉 👍