chai-as-promised broken by change to addMethod and addProperty

[meeber]

@keithamus @lucasfcosta @domenic

A PR submitted back in March (#642) seems to break chai-as-promised. The PR changed addMethod and addProperty so that it returns a new assertion with flags transferred over instead of returning this. However, it seems that chai-as-promised relies on the previous behavior so as not to lose the promise throughout the chain.

I haven't yet dived in deeply enough to know if this is in an easy fix (for chai or for chai-as-promised). Just raising awareness.

[lucasfcosta]

Hmm, since I sent that PR I will make sure to take a look at it too next week (probably this weekend, but I cannot guarantee).
Thanks for detecting this! Good job.

[meeber]

This problem can be resolved from within chai by adding chai-as-promised's transferPromiseness logic before the return newAssertion in addMethod and addProperty:

var newAssertion = new chai.Assertion();
transferFlags(this, newAssertion);
if ("then" in this) newAssertion.then = this.then.bind(this);  // Resolves the problem
return newAssertion;

But I haven't found a way to resolve it from within chai-as-promised.

Edit: Opened PR with possible solution

[lucasfcosta]

@meeber I think it would be better to solve this in chai-as-promised since Chai is a "father" for other modules, I think they're the ones that should follow the changes on Chai's core.

Great work tracking this down, I hadn't had enough time to check this on the weekend, college's last weeks are driving me crazy, however I took some time today to investigate this and left a comment at your PR on chai-as-promised.

Anyway, good job!

[domenic]

I'm not sure we should be making changes in Chai as Promised to compensate for Chai breaking backward compatibility in a non-major version.

[keithamus]

@domenic I agree with your sentiment - I wonder what the most pragmatic thing to do here is though? We're right around the corner from [email protected]. We could try to patch over this problem in chai for now, but I think any code which would patch the problem would probably be ugly and likely be removed for 4.0 - meaning chai as promised would need this code to support 4.0 anyway. Thoughts?

[keithamus]

As an aside to spike a bit of discussion - I think we should look at somehow pulling in a handful of plugins as part of chai's test suite, to stop or at least notify us of changes to Chai that break plugin compat.

[lucasfcosta]

@keithamus Great idea, I'd love to have that.
However I think it should not be part of the build process. The main goal of those tests should be to inform us which plugins will break before a release and then contact/help the plugin's developer for a fix before the release.

I also have a doubt: although that PR was merged on master it will just be out in the wild when we release 4.0.0 right? Or do we have plans for a non-major version before that? If this gets released only on 4.0.0 we would be doing a breaking change only on this release and therefore we would still be following SemVer, wouldn't we?

[keithamus]

I also have a doubt: although that PR was merged on master it will just be out in the wild when we release 4.0.0 right

Yeah you're right actually. I was confused and thought #642 was part of 3.5.0 but it isn't. It will be part of 4.0.

[keithamus]

So just to reiterate to @domenic - this change will actually be part of a breaking change in 4.0.

[domenic]

OK, great! Happy to accept fixes to Chai as Promised once Chai 4.0 is released, but it's good to know no existing users who respect the peer dependency warnings will be broken.

[meeber]

Yup, just wanted to make sure we had this ironed out prior to 4.0 release, sorry for the confusion.

We have a WIP PR going on at chaijs/chai-as-promised#157 for discussion.

[meeber]

Closing this in favor of chaijs/chai-as-promised#157