Marionette 2.0 - isEmpty doesn't use the collection it is passed

[MeoMix]

    // check if the collection is empty
    isEmpty: function (collection) {
        return !this.collection || this.collection.length === 0;
    }

line 2071

[joshbedo]

👍
I didn't even know you could pass the collection into the isEmpty() method. I use it mainly like view.isEmpty() I'm not sure when you would want to do view.isEmpty(collection)

[samccone]

Me either @joshbedo alas need to fix this one

[jamesplease]

Just fyi there are tons of methods like this in Marionette. I chose not to change it in v2 because it's potentially breaking, and affects so much of the codebase.

We should make a single issue to track all of them.

[MeoMix]

I made a quick scan of Marionette (visual studio highlights unused JavaScript variables) and created issues for the ones I saw. There weren't tons, only a handful, but perhaps others using variables but not correctly.

[samccone]

"Fixing" this method would be breaking if someone is passing random data to the method and all of a sudden we start returning the correct value.

Currently the implementation contract and the documentation are out of sync.

The fix could be simply to X the argument

[joshbedo]

Couldn't something like this potentially be done? That way the old way and correct way both work. I personally don't think anyone should ever pass a collection as an argument.. thats what utility methods are for. If it was _.isEmpty() that would be understandable.

    isEmpty: function (collection) {
      col = collection || this.collection  
      return !col || col.length === 0;
    }

[cmaher]

+1 to removing the argument. I don't see any benefit to implementing new behavior just because the signature is weird.

[samccone]

👍

[joshbedo]

@cmaher agreed, nobody uses collection as an argument anyway and if they do they should stop :)

[jamesplease]

Related: #930

[jamiebuilds]

👍

[samccone]

The fix for these is to add Documentation around why the argument is there.

[cmaher]

Every time this is called, the CollectionView passes this.collection to it. Is this something that should be removed in v3/4 and documented for now?

[samccone]

yep @cmaher totally!

PS not sure if we ever want to actually remove it because it is useful if you are overriding the method

[jamesplease]

PS not sure if we ever want to actually remove it because it is useful if you are overriding the method

I think @cmaher's point is that the method will always be called from the collection view itself, so you will have access to this.collection and every other property from within the method. No need to pass it in.