Filtering collections isn't working properly

[msl-kabo]

Code:

          account.getGroupMemberships({expand: 'group'}, (err, memberships) => {
            memberships.filter((membership, next) => {
              console.log('checking if membership should be included', membership.group.name);
              const test = membership.group.name.startsWith('myString');
              console.log('will be included?', test);
              next(null, test);
            }, (err, memberships) => {
              console.log('got filtered group memberships', err, memberships);
            });
          });

Output:

checking if membership should be included myString:831f9e62cd
will be included? true
checking if membership should be included myString:831f9e62cd/example:34a800114c
will be included? true
checking if membership should be included stuff:831f9e62cd/example:34a800114c
will be included? false
got filtered group memberships [] undefined

I would expect err to be undefined and memberships to contain the two first memberships. Documentation: https://docs.stormpath.com/nodejs/jsdoc/CollectionResource.html#filter__anchor

Using stormpath 0.18.5 on node 6.9.1.

[msl-kabo]

Doing .map instead of .filter and returning false if the element shouldn't be included (and then checking for false in subsequent operations) works fine, so I'm able to work around it, but annoying.

[robertjd]

Hi @msl-kabo , thanks for reporting this, I can confirm the same bug. It seems that we aren't actually respecting the next(err, test) signature, instead we're looking for next(test). We want the former, as we intend for this iteration functions to work just like their async library counterparts, e.g. http://caolan.github.io/async/docs.html#filter

We'll get this fixed, my apologies for the frustrations.

[the-overengineer]

Turns out it was actually not a bug per se. Instead, the documentation was incorrect. We were using asyc@1.5.x and the CollectionResource method documentation (and your link) matches async@2.x.y. Filter really does work with next(test) and done(val) in 1.5.x, like one can see here.

PR #570 updates async to 2.1.x to match our docs and the saner (more uniform) interface, and makes our code and the tests play nice with it.

Please note that this is a breaking change in the interface, however.