New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.length(value) deprecation #684
Comments
Also, why is |
Hi guys, thanks for the issue! @ljharb @lencioni Please let me know if you guys want to know anything else 😄 |
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 |
|
@shvaikalesh well noticed! |
I have a couple of concerns with this. My first concern is that 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 Thanks to @lucasfcosta's work in #642, this is now only an issue when One consequence of removing Another consequence is that the replacement Finally, completely removing 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:
|
@meeber agreed! Great points, I'm convinced. Also, we can study if there is any JavaScript jiu-jitsu we can use to avoid inheriting |
Not possible per spec, every callable object (function) has own |
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 |
A codemod should be shipped to support breaking changes like this. That will make the upgrade process significantly less painful. |
There's a small benefit to removing What I'm suggesting is that, given Chai's maturity, there is now a cost to removing |
The most annoying thing about this all is that, while ES5 makes |
Given that But if I'm overruled, then I do insist we remove it entirely (instead of just the assertion version). |
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. |
I'm going to re-open this and label as |
I agree, and if the cost of removing |
Since discussion restarted on this in #841, now seems like a good time to make a decision regarding The problem is that 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 Options:
Let's figure out where everyone stands on this issue and move forward with an option for 4.0. |
1 > 3 > 4 > 2 |
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 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
To misquote Lt James Gordon, it's not the fix we deserve but it is the fix we need. |
or |
Thanks @meeber for moving this forward. Option 3 seems the best to me, given the circumstances. |
Just a suggestion, why not make What I mean is, is the issue with |
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 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 @keithamus suggestion seems to be the optimal one. |
@leggsimon The issue is with 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. |
@ljharb we already have 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 ❤️ |
@keithamus Yup, I vote for option 3. |
- The method part of the `length` assertion was slated for deprecation due to a compatibility issue in legacy environments. The decision to deprecate `length` was reversed per chaijs#684. This commit replaces the deprecation notice with a recommendation to favor `lengthOf` over `length` due to the compatibility issue. - The `lengthOf` assertion wasn't a true alias of `length` because it was a method assertion instead of a chainable method assertion. This commit changes `lengthOf` into a chainable method assertion that's identical to `length`, and updates tests and docs accordingly. - Updates docs to classify `length` as an alias of `lengthOf`. - Updates docs of related assertions to use `lengthOf` instead of `length`.
- The method part of the `length` assertion was slated for deprecation due to a compatibility issue in legacy environments. The decision to deprecate `length` was reversed per chaijs#684. This commit replaces the deprecation notice with a recommendation to favor `lengthOf` over `length` due to the compatibility issue. - The `lengthOf` assertion wasn't a true alias of `length` because it was a method assertion instead of a chainable method assertion. This commit changes `lengthOf` into a chainable method assertion that's identical to `length`, and updates tests and docs accordingly. - Updates docs to classify `length` as an alias of `lengthOf`. - Updates docs of related assertions to use `lengthOf` instead of `length`.
I noticed that the documentation says:
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?The text was updated successfully, but these errors were encountered: