an language chain affects length language chain?

[benmosher]

Chai 3.4.1, Node 4.2.2:

expect([1, 2, 3]).to.be.an.instanceOf(Array).and.to.have.length.below(1000)

... should work, but fails because length returns 0.

Removing an from the chain, leaving:

expect([1, 2, 3]).to.be.instanceOf(Array).and.to.have.length.below(1000)

works as expected.

Seems similar to an issue reported late in #427 by valscion.

[keithamus]

Hey @benmosher sorry for the late reply.

You're totally right this does seem to be an issue. It looks as though an, being an actual assertion function, overrides the length property (because its a function). And so length is 0 in the first example, while in the second it is the assertion.

This seems to be to do with how the chain is formed. addChainableMethod does some special magic to return both the method and the chain.

All normal methods just return this - which obviously changes between addChainableMethod and addMethod. To illustrate:

> chai.expect([1, 2, 3]).to.be.an.instanceof(Array)
[Function: assert]
> chai.expect([1, 2, 3]).to.be.instanceof(Array)
Assertion {
  __flags: { ssfi: [Function], object: [ 1, 2, 3 ], message: undefined } }

I'm not entirely sure how we can fix this, certainly without sweeping architectural changes. We could change how addMethod works by, instead of returning this, returning a new Assertion chain with the flags copied over - for example L42 would have to become:

return result === undefined ? transferFlags(new chai.Assertion()) : result

I'm not entirely sure that would work though. It'd certainly need lots of extra tests to back it up.

I'll mark this as pull-request-wanted and hard-fix for now - if you want to explore the above solution in a PR, it'd very much be welcome. If it doesn't work then we can revisit this issue and look for other solutions.

[benmosher]

Fair enough. I'm not sure if I'll have bandwidth to attack this, especially given the simplicity of the workaround (just don't chain a/an with length).

Mostly wanted to get it documented for others, as it was pretty surprising when one of my teammates stumbled onto it.

[keithamus]

Totally understandable. I'll leave it open should you or anyone else want to tackle it.

[lucasfcosta]

I'm gonna take some time this week to take a look at this and report back as soon as I've explored possible solutions.

I'm also available to help if anyone is already working on this (and if they do want help, of course).

[ghost]

I'm having this problem, too.

This is broken:

expect(my_object).to.have.a.deep.property("foo.bar").that.is.an("array").with.a.length.of.at.least(1);

However, this works (and is the workaround I'm using for now):

expect(my_object).to.have.deep.property("foo.bar").that.is.an("array").with.length.of.at.least(1);

Curiously, if either the a on the deep property check or the length check is present, the test fails. This surprised me since I figured only the one at the end would have mattered!

[benmosher]

FWIW, I've just started using .property('length') everywhere, instead of .length.

I might even go so far as advocating the deprecation + removal of the length shortcut as a solution to this, instead of trying to work out a workaround.

[benmosher]

Actually, just noticed this in the API docs:

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.

What happened to this?

[keithamus]

@benmosher that notice is slightly different - that refers to calling length like a method (e.g. length(0)). The theory is that length as a keyword (e.g. length.of.at.least) should still work, although there are some rather unfortunate edge cases, as @ironwallaby pointed out.

[lucasfcosta]

Hello guys, I've just fixed this thanks to @keithamus idea.

I followed his suggestions (which were great by the way, because it enabled us to keep things simple) and made some (very) minor adjustments.

I will raise a PR as soon as I've got some tests to cover it up.

[benmosher]

Ahhhh, that makes sense, thanks. 😄

[lucasfcosta]

Note: This has been partially solved at #642. Please read that PR for more details.

---

@keithamus should we close this or not, as it has been partially solved?

[keithamus]

I think this has been as solved as we can for now, so we'll close it 😄