Chai: .length(value) deprecation

[lencioni]

I noticed that the documentation says:

Deprecation notice: Using length as an assertion will be deprecated in version 2.4.0 and removed in 3.0.0. Code using the old style of asserting for length property value using length(value) should be switched to use lengthOf(value) instead.

But I also noticed that chai is on 3.5.0 and .length(value) seems to still be a thing. Am I missing something here?

[ljharb]

Also, why is .to.have.length deprecated at all?

[lucasfcosta]

Hi guys, thanks for the issue!

@ljharb
I hope @keithamus will correct me if I'm wrong, but I think this is related to #642.
When calling .length after using an a assertion we get back the .length property of a function instead of the .length property of the Assertion prototype and this causes bug, because people expect it to be the length assertion and not a function's length property.
If you want more details on it you can take a look at #642, especially at this post.

@lencioni
What's been deprecated is using length as an assertion.
At this issue you can read more about this deprecation. It includes an explanation by @keithamus on this matter. I think it may solve your doubts.

Please let me know if you guys want to know anything else :smile:

[keithamus]

Yeah, essentially there are some edge cases around the fluent interface - where chaining functions results in the return value being a function, not the instance object. Ultimately this means _sometimes_ length is Function#length not our assertion. Rather than detailing exactly where this happens, it's much easier to just deprecate length and instead use lengthOf.

[shvaikalesh]

4.0 is coming, but .length as an assertion is not yet removed. At least, I think, we should remove it from homepage. Any way I can help?

[lucasfcosta]

@shvaikalesh well noticed!
I think we should take advantage of 4.x.x and remove that from the code as well.
What do you guys think?

[meeber]

I have a couple of concerns with this.

My first concern is that length as an assertion should only be deprecated if length as a flag-setting language chain is also deprecated. The same issue affects both variations so it would be a weird user experience to deprecate one but not the other:

expect("blah").to.have.a.length(4);  // fails
expect("blah").to.have.a.length.within(3, 5);  // fails

My second concern is that I'm not sure that removing length gives enough benefit to make it worth doing.

Thanks to @lucasfcosta's work in #642, this is now only an issue when length is chained directly with a method, without anything in between, most notably with have.a.length. This is annoying to be sure, but it seems low impact, especially since it only results in a somewhat vague error message. At least it's not causing tests to wrongly pass or any other kind of silently destructive behavior.

One consequence of removing length completely is that many Chai examples on the net will no longer work. The confusion experienced by new developers trying to use length based off such examples (or off of intuition) may be greater than they would experience in the event that they tried using have.a.length and didn't understand the error message.

Another consequence is that the replacement lengthOf doesn't work grammatically in many situations as a replacement for the length language chain, so an additional alias such as len would need to be added, which isn't nearly as intuitive as length.

Finally, completely removing length will mean people will have to update a lot of valid tests in order to upgrade Chai. Sometimes this is inevitable with breaking changes but is it really worth it for this?

It's also worth noting that we may have better alternatives available to us if we can stomach a divergence in behavior between legacy and modern environments, in much the same way as exists now with the new proxy behavior. Such alternatives include:

• Detect if the .length property is configurable on functions and if so delete it whenever creating a new method.
• Expand functionality of proxy-protection so that length gives a descriptive error message whenever it's chained off a function. On the plus side, the behavior will remain the same across legacy and modern environments so developers won't need to account for different environments when writing tests, the only difference being that modern environments will benefit from a more descriptive, developer-friendly error message.

[lucasfcosta]

@meeber agreed! Great points, I'm convinced.
Also, I think that the first suggested fix is more adequate for our situation, because the browsers that support proxy are almost the same ones that have the length property as configurable.
Now that @meeber pointed these issues I think the tradeoff isn't worth it for us to deprecate length.

Also, we can study if there is any JavaScript jiu-jitsu we can use to avoid inheriting length but still be able to invoke functions, but I'm not sure it will be possible.

[shvaikalesh]

Also, we can study if there is any JavaScript jiu-jitsu we can use to avoid inheriting length but still be able to invoke functions

Not possible per spec, every callable object (function) has own length. The only exception that comes to mind is document.all.

[ljharb]

This is just an inherent problem when trying to make this kind of fluent interface in JS. I think it would be best to get rid of length in all its forms.

[lencioni]

completely removing length will mean people will have to update a lot of valid tests in order to upgrade Chai. Sometimes this is inevitable with breaking changes but is it really worth it for this?

A codemod should be shipped to support breaking changes like this. That will make the upgrade process significantly less painful.

[meeber]

There's a small benefit to removing length, and if Chai were a young project, there would be virtually no cost in doing so, making it the clearly correct choice.

What I'm suggesting is that, given Chai's maturity, there is now a cost to removing length, and that cost is now greater than the benefit.

[keithamus]

The most annoying thing about this all is that, while ES5 makes length a non configurable property, in ES6 it is configurable - meaning _we could have length_ if we only supported ES6 environs. Maybe we could just bury our head in the sand for a couple of years until everyone is on es6 😜

[meeber]

Given that length is only broken when it follows a function (i.e., a), and that we can easily leverage it's ES6 configurability to make it so that it's only broken in legacy environments (two of which we're soon dropping support of: node v0.10 and v0.12), and that it's only broken in the sense that it gives an error message when used in that one situation (instead of something more harmful), I think it's the correct cost-benefit decision to just let it slide. So that's my vote.

But if I'm overruled, then I do insist we remove it entirely (instead of just the assertion version).

[keithamus]

I'm not sure how I feel about supporting some parts in some browsers, and other parts in others. I suppose we already do this, but I feel like we do it in fairly transparent ways. I just envision getting bug reports (or forcing people to sit down for some significant portion of time to figure out) why their assertion is working in one browser but not another. Or to rephrase that - developers (me included) have a hard enough time debugging cross browser issues, I certainly don't want chai to be dogpiling on that.

[keithamus]

I'm going to re-open this and label as more-discussion though, as it's obviously unresolved.

[meeber]

I'm not sure how I feel about supporting some parts in some browsers, and other parts in others.

I agree, and if the cost of removing length was very small, such as at the beginning of the project, it'd be the way to go. But given the present situation, and given how minor of a bug this is, I think a temporary divergence in behavior between modern and legacy environments is the lesser of two evils when compared to feature removal.

[meeber]

Since discussion restarted on this in #841, now seems like a good time to make a decision regarding .length. Here's a summary of the problem and our options:

The problem is that .length throws an error when chained directly off of an uncalled chainable method (e.g., .a.length). In the past, it threw errors in more situations, but #642 narrowed it down to only this one situation. The reason this problem exists is because .length is a builtin property of functions in JavaScript, and the reason it can't be fixed by just deleting the builtin .length property and replacing it with Chai's .length is because in legacy environments such as Node v0.12 and IE 11, the .length property is non-configurable.

It's worth noting that no issues regarding this problem have been opened on Chai's issue tracker since #642 was implemented; all discussions about it have instead been triggered by someone asking about why .length is being deprecated. I consider it a low impact problem, both because it's rarely triggered, and because it fails loudly by throwing an error message instead of doing something more destructive, such as silently passing an invalid assertion.

Options:

1. Completely remove all forms of .length from Chai. From a purely ideological standpoint, I think this is the best option, and it would've been my preferred option near the beginning of the project. However, at this point in the project, I believe that this ends up being a high-impact solution for a low-impact problem. In other words, the cost of this breaking change would be far greater than the benefit of fixing the bug, even if a codemod was shipped to help users update their assertions. Therefore, I'm opposed to this option.

2. Fix the bug in ES6 environments (meaning it will still fail in legacy environments). I'm no longer in favor of this option. The same tests having different results between different environments supported by Chai is too costly to some users, even though it fixes the bug for the majority of users.

3. Use proxies to provide a more user-friendly error message in ES6 environments (meaning it will still fail in all environments). This is my favorite option. The behavior remains consistent among all environments given that an error is always thrown, but in most environments we're able to provide a more descriptive error message to our users. And in the future when we no longer support pre-ES6 legacy environments, we can circle back to this issue and fix it for good by implementing Option 2.

4. Do nothing. Given that Option 3 doesn't have any cost to users, I don't see any advantage to this option over Option 3. Therefore, I'm opposed to this option.

Let's figure out where everyone stands on this issue and move forward with an option for 4.0.

[ljharb]

1 > 3 > 4 > 2

[keithamus]

Well articulated @meeber. I've been putting thought into this myself and came up with the same conclusion as I think you have: I think door number 3 - explicitly erroring in <function>.length scenarios for ES6 to highlight problems on both ES6 & ES5 environments.

As you mentioned above, the only real case for this should be where a method assertion is used right before a keyword - for chai core this is only a/an but we could probably detect whatever method it was, and provide an appropriate error message...

Error: you used `.have.a.length(1)` but due to an edge case in older browsers `a.length` won't work. Please use `.have.length(1)` instead.

To misquote Lt James Gordon, it's not the fix we deserve but it is the fix we need.

[ljharb]

or .have.lengthOf(1)?

[vieiralucas]

Thanks @meeber for moving this forward.

Option 3 seems the best to me, given the circumstances.

[leggsimon]

Just a suggestion, why not make a/an only language chains and have make an ofType to replace their current type checking usage?

What I mean is, is the issue with length or with a/an?

[lucasfcosta]

Option 3 seems the best to me too.

As I said in #841, I initially thought it could be a good idea to add some warnings to everyone using length, but as @meeber said, there are lots of examples using length on the internet and there's no great benefit in deprecating it.

IMO it's better to keep behavior consistent and warn our users whenever we need to. Also, I think that everyone testing on IE11 and other browsers will also test their code on other more modern environments thus they'll be aware they shouldn't use a.length due to the helpful error messages provided in other environments.

@keithamus suggestion seems to be the optimal one.

[meeber]

@leggsimon The issue is with .length. I believe that a.length is the most likely way a user would trigger the bug, but if .length follows any of Chai's uninvoked chainable methods, then the same thing would happen.

Regardless, my primary argument is that the bug is so low-impact that any breaking change that fixed the bug would result in a bigger negative impact on users than the actual bug is having.

[keithamus]

@ljharb we already have lengthOf as an alias to length. You can even say .have.a.lengthOf. The contention lies in whether we deprecate length _in favour of_ lengthOf, or keep length but have some pitfalls with it.

It seems like we're mostly settled on option 3? From your post @meeber I assume you are also happy with 3? Shall we progress with outlining what needs doing to put option 3 in motion?

I want to echo @vieiralucas's sentiments too - @meeber you've been doing some excellent work with all this analysis (and in the other issues too) so I consider us very lucky to have you working on chai! Much ❤️

[meeber]

@keithamus Yup, I vote for option 3.