Detecting whether a method is chainable (and general concerns with breaking changes)

[TimothyGu]

When I tried to upgrade node-fetch to Chai v4.x, the following error popped up:

  1) node-fetch should return a promise:
     AssertionError: expected undefined to be an instance of Promise

The vagueness of this message prompted me to dig deeper, and the cause of this error can actually be attributed to a single commit. ae69f27 changed lengthOf to be a chainable method rather than a vanilla method. This broke chai-iterator plugin, which assumes lengthOf to be a vanilla method.

This brings up several questions:

• Why was the change in semantics of lengthOf not part of the official release notes?

  In fact lengthOf appeared only once in release notes, in the one for 4.0.0-canary.2, hardly a version one would check to upgrade to v4.x, under "Bug Fixes":

  We've updated documentation and error handling around using .length (Un-deprecate length and add guard #897) - which can, in some cases, be problematic. Typically, you'll want to use .lengthOf instead - but the documentation now makes this clear, and errors are more helpful when bad things happen.

  Given the breakages this change can cause, the change (or rather addition) in semantics should be highlighted at least in "New Features", if not "Breaking Changes", in the release notes for 4.0.0.

• Could utils.overwriteMethod be made to work with chainable methods?

  This is probably most pertinent to the actual issue. If utils.overwriteMethod works with chainable methods the bug wouldn't have occurred. But even if it is technically impossible to make it work, it should at least throw an error warning developers that something's off. Which gives rise to the next question:

• Could there be a publicly-exposed method for checking the type of method?

  This would allow plugins like chai-iterator to support multiple versions of Chai simultaneously.

[meeber]

Hey @TimothyGu, sorry for the issue here.

Why was the change in semantics of lengthOf not part of the official release notes?

That's my mistake for incorrectly classifying ae69f27 as a "refactor"; as you've exemplified, this change is breaking for at least one plugin, and it should've been classified as such. I'm guessing its classification is why it got overlooked when writing the release notes. And it wasn't caught during the many months of testing, including testing against popular plugins per #890, some of which overwrite lengthOf.

Could utils.overwriteMethod be made to work with chainable methods?
Could there be a publicly-exposed method for checking the type of method?

Chai is old and there are lots of architectural problems with it. The entire plugin system needs to be rewritten (see #585). These kinds of issues will be fixed as a part of that. I'm not opposed to adding some kind of sanity checking to the current implementation (if a PR was submitted), but this hasn't been a common issue, so I'm not sure it's worth the cycles.

[TimothyGu]

@meeber Thanks for the clarification. The working you are doing in #890 is impressively thorough, and it does seem like this issue is the unfortunate exception rather than the norm in Chai development.

For now, we'll stay at Chai 3.x, and I'll make a PR to chai-iterator upgrading it to be compatible with Chai 4.x. I'd like to applaud your efforts in maintaining such legacy-heavy framework as Chai, and will now close this issue. Thank you!