ENikS/LINQ

GroupBy doesn't work if key is empty string or null

Closed this issue · 4 comments

if the key returned by keySelector is empty string or null, GroupBy ignores it.

utilities.ts

function getKeyedMap(), getKeyedMapFast(), getKeys()
let key = keySelector(value);
if (!key) continue;

this behavior is incorrect since the key should be according to application business logic in which empty string or null can be valid.
(the internal implementation using Map or Set supports it)

Moreover, according to the code any falsy key is ignored (0, false, '', null, NaN)
Which are valid keys as well.

PS. in C# Linq having key as empty string or null is supported
PS2. the issue discovered while migrating from linq-es5 to linq-es2015

Please support key as empty string, null or other falsy value

thanks

ENikS commented

Could you create a C# sample to verify your issue? Just a simple unit test will do.

C# sample of GroupBy with empty string and null.
[ Can be tested here: https://dotnetfiddle.net/ ]

using System;
using System.Linq;

public class Program
{
    public static void Main()
    {
        var expectedKeys = new [] 
        {
          new {Key = null as object}, new {Key = "" as object}, new {Key = 0 as object}, new {Key = false as object}
        };
        
        var actualKeys = expectedKeys
            .GroupBy(item => item.Key)
            .Select(group => group.Key)
            .ToArray();

        System.Diagnostics.Debug.Assert(actualKeys.Length == expectedKeys.Length);
        foreach(var expected in expectedKeys)
        {
            System.Diagnostics.Debug.Assert(actualKeys.Contains(expected.Key));
        }
    }
}

PS. in the project, under test/data.ts
if variable undef would be part of pets array it could cover test case of key (pet.age) is zero

var undef : any = { Name: "Puppy", Age: 0, Owner: undefined };
//   undef  should go into pets array ?
export var pets   = [barley, stray, boots, whiskers, daisy];
ENikS commented

If you are so deep in it, why not send a pull request with the fix and tests to cover these?

ENikS commented

Deployed new package. Thank you for your help!