electricessence / TypeScript.NET

A JavaScript-Friendly .NET Based TypeScript Library (Moved)
https://github.com/electricessence/TypeScript.NET-Core
Other
251 stars 36 forks source link

Group bug? (version 2.1.0 from nuget) #24

Closed JumpNRun closed 8 years ago

JumpNRun commented 8 years ago

Hello, first of all i want to thank you for this great piece of work :) I'm using this in one of my current projects in a playground and found a bug? while playing around. I'm not sure if it really is a bug, it might be also just a misunderstanding of how this library works.

var objArray = [
    { Name: "John", Id: 0, Salary: 1300.00, Company: "Microsoft" },
    { Name: "Peter", Id: 1, Salary: 4800.50, Company: "Microsoft" },
    { Name: "Sandra", Id: 2, Salary: 999.99, Company: "Microsoft" },
    { Name: "Me", Id: 3, Salary: 1000000000.00, Company: "Hell Corp." }
];
var groups = Linq.from(objArray).groupBy(x => x.Company, x => x);
var companies = groups.select(x => x.key).toArray();
console.log('Companies: ' + companies.toString());

This works fine. But if i want to access the individual groups it doesn't work as i expected.

groups.forEach(x =>
{
    var enumerable = new Linq.Enumerable(x.getEnumerator);
    console.log(x.key + ': ' + enumerable.sum(x => x.Salary).toLocaleString())
});

This code in the second line enumerable.sum() causes this error: "too much recursion". It looks like all methods throw this error. But this works:

groups.forEach(x =>
{
    var sum = 0;
    var enumerator = x.getEnumerator();
    while (enumerator.moveNext()) {
        sum += enumerator.current.Salary;
    }
    console.log(x.key + ': ' + sum.toLocaleString())
});

I'm coming from C# and the first option looks more intuitive. If this is is working as intended this might be a good thing to change? :)

Edit: I tested this in Firefox 41.0.2 In Chrome 46 i get this error: "Uncaught RangeError: Maximum call stack size exceeded"

electricessence commented 8 years ago

Investigating.

electricessence commented 8 years ago

Also, FYI, the .nuget is out of date. :( I'll try to get it updated asap. I suggest pulling from NPM until then.

electricessence commented 8 years ago

Ok right away this seems weird:

groups.forEach(x =>
{
    var enumerable = new Linq.Enumerable(x.getEnumerator);
    console.log(x.key + ': ' + enumerable.sum(x => x.Salary).toLocaleString())
});

Should be

groups.forEach(g =>
{
    console.log(g.key + ': ' + g.sum(x => x.Salary).toLocaleString())
});

There is an issue you exposed where "IGrouping" is not inheriting from Enumerable so it doesn't expose all the methods and makes it seem inaccessible even though it is. .....

Just pushed a fix for this to the master branch.

electricessence commented 8 years ago

9859f1b9ca638e57242e82347c15a50575c126d6 2ae7aa4402775af9a331410fa994017e58b6eab0

Also fixed an issue with groupBy where the signature wouldn't infer types correctly. You no longer need to put x=>x after the selector.

electricessence commented 8 years ago

@JumpNRun ... Here's the latest .nuget package... The AMD version will be removed eventually. https://www.nuget.org/packages/TypeScript.NET.Library