Consistency with returning new Assertion instead of this

[meeber]

A change was made a few months ago to return a new Assertion object with flags copied over instead of returning this. References: #562 #642

However, it looks like there are still some spots where this is returned. The only one I've actually tested so far is overwriteProperty.

For example:

const {expect, use} = require("chai");                                                                                  

use(function (chai, utils) {                                                                                            
  utils.overwriteProperty(chai.Assertion.prototype, "and", function (_super) {                                          
    return function blah () {                                                                                           
      _super.call(this);                                                                                                
    };                                                                                                                  
  });                                                                                                                   
});                                                                                                                     

it("but", () => {                                                                                                      
  expect([1, 2, 3]).to.be.an.but.length.below(1000)    // Passes
});

it("and", () => {                                                                                                      
  expect([1, 2, 3]).to.be.an.and.length.below(1000)    // Fails
});

As expected, the second assertion fails with error: "TypeError: expect(...).to.be.an.and.length.below is not a function" because the overwritten and is returning the function that was originally returned by the chainable method an instead of returning a new Assertion.

Other places where this might be a problem (but I haven't tested):

• overwriteMethod
• addChainableMethod
• overwriteChainableMethod

[lucasfcosta]

Well noticed @meeber, thanks for the awesome job! 😄
This really seems to be a problem, I feel like we need to solve it before releasing 4.0.0 just to keep the API consistent.
I think this will be a pretty easy thing to fix but it will require some time to check if any other places need a fix too.
Right now I'm finishing the 4.x.x migration guide and I'm going to review the merge you have done, but after doing that I may be able to fix it. If any contributor wants to give it a shot please feel free to do so, I will make sure to post another comment here if I start doing anything related to this.

[meeber]

Interestingly, I just discovered that this issue is blocking us from adding proxy support to overwritten properties. When a overwritten property returns this, it's the non-proxified version. Would need to either wrap this in a proxy or return a new Assertion like the addProperty function does.

[meeber]

Also, I've always wondered about this part of the logic: In what situation is result not undefined?

[meeber]

Hey @vieiralucas welcome aboard!

NOW FIX THIS!

[vieiralucas]

YES SIR! 😸

I'm still reading through all the issues and PR's involved in this issue.
I think I'll be able to submit a PR addressing thi later today.

[lucasfcosta]

@vieiralucas This PR #642 has more details about it. I think this post explains the reason behind this whole thing, I highly recommend anyone involved with this issue to read that.