There's an inconsistency with Lo-Dash and Underscore: <div class="highlight highli

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

Moving sortBy into the collections category about lodash HOT 23 CLOSED

lodash commented on April 27, 2024

Moving sortBy into the collections category

from lodash.

Comments (23)

commented on April 27, 2024

It's certainly a useful behavior—but, unfortunately, object enumeration semantics are implementation-dependent, so there's no guarantee that your object properties will actually be sorted.

Update: That said, this is a backward-compatibility issue that merits further discussion. @jdalton, @mathiasbynens?

from lodash.

jeremyckahn commented on April 27, 2024

Perhaps I'm misunderstanding you, but sortBy should still iterate through all of the properties in the input Object. In Underscore, sortBy returns an array with elements ordered by an iterator function. It's been reliable for me in practice, and it would be great if Lo-Dash supported this as well.

from lodash.

commented on April 27, 2024

@jeremyckahn Right, but I don't think it makes much sense to sort an object (given that the order of the properties isn't defined to begin with).

In other words: console.log(sorted) may produce either [ { prop: 1 }, { prop: 2 } ] or [ { prop: 2 }, { prop: 1 } ]. Both results are equally valid according to the spec.

from lodash.

jeremyckahn commented on April 27, 2024

The sample code I posted in my original message produces consistent results in Chrome and Firefox, but it's worthwhile to put together a unit test to confirm the ordering with different interpreters. I can put one together in the next day or so.

Aside from that, I'll just reiterate that I feel that this is really useful functionality in Underscore, and I'd love it if Lo-Dash supported it.

from lodash.

commented on April 27, 2024

@jeremyckahn Thanks! If the ordering turns out to be nothing more than an academic concern, we can definitely support this in Lo-Dash!

from lodash.

jeremyckahn commented on April 27, 2024

Here's a quick and dirty unit test. I get consistent results in Chrome, Firefox, and IE7/8.

<!DOCTYPE html>
<html>
<head>
  <link href="http://rekapi.com/lib/shifty/tests/qunit/qunit.css" rel="stylesheet" type="text/css" />
  <script src="http://rekapi.com/lib/shifty/tests/qunit/qunit.js"></script>
  <script src="http://underscorejs.org/underscore-min.js"></script>
  <script>
    (function () {
      module('Underscore sortBy ordering');

      test('Objects are sorted into Arrays', function () {
        var sorted = _.sortBy({
          a: { prop: 2 },
          b: { prop: 1 }
        }, function (obj) {
          return obj.prop;
        });

        equal(sorted[0].prop, 1, 'First property was sorted correctly.');
        equal(sorted[1].prop, 2, 'Second property was sorted correctly.');

      });
    } ())
  </script>
</head>
<body>
  <h1 id="qunit-header">Underscore sortBy test</h1>
   <h2 id="qunit-banner"></h2>
   <div id="qunit-testrunner-toolbar"></div>
   <h2 id="qunit-useragent"></h2>
   <ol id="qunit-tests"></ol>
   <div id="qunit-fixture"></div>
</body>
</html>

from lodash.

commented on April 27, 2024

Thanks for doing this, Jeremy—much appreciated!

from lodash.

jdalton commented on April 27, 2024

Chrome and Opera sort numeric properties, which is inconsistent with other browsers:
http://jsbin.com/ejecor

@jeremyckahn I've tried to find a sweet spot between tested/documented/common use. I moved sortBy and groupBy to the "Arrays" category because its documented example and tested usage was for arrays.

from lodash.

jdalton commented on April 27, 2024

@jeremyckahn
As more devs use Lo-Dash we will get a better feeling on how the API is actually used. I'm not against moving groupBy and sortBy back to the "Collections" category.

@kitcambridge
Object iteration issues seem like a separate concern from what groupBy is doing as it's grouping and not sorting.

from lodash.

jeremyckahn commented on April 27, 2024

@jdalton I'm happy to throw in my two cents, as I feel that this is a really valuable project.

I'm curious, is there a significant performance concern with moving sortBy into the Collections category?

from lodash.

jdalton commented on April 27, 2024

@jeremyckahn

I'm curious, is there a significant performance concern with moving sortBy into the Collections category

Naw, not really a big perf hit, it just keeps things simpler but it's easy enough to add back ;)

I'm curious do you use _.max and _.min with objects too?

from lodash.

jeremyckahn commented on April 27, 2024

I haven't, but I can see as much value in using max and min on Objects as I do with sortBy.

from lodash.

jdalton commented on April 27, 2024

@jeremyckahn

What do you think of _.shuffle supporting objects?

from lodash.

jeremyckahn commented on April 27, 2024

I think shuffle is fine with just operating on Arrays. The order of the elements in an Object are already random, so making them a different kind of random doesn't add much to the library.

from lodash.

jdalton commented on April 27, 2024

The code needed to get this to work requires adding more method calls, and dependencies. You should rethink your animation framework's approach. Iterating over object properties is a pretty lame way to go about it. That said, I am patching it.

from lodash.

jdalton commented on April 27, 2024

You could also try _.pluck(collection, 'prop').sort().

from lodash.

jeremyckahn commented on April 27, 2024

I'm curious as to why you say "iterating over object properties is a pretty lame way to go about" calculating state for animation. Could you please elaborate?

from lodash.

jdalton commented on April 27, 2024

@jeremyckahn Animation frameworks should be fast and iterating over objects via a for-in loop is pretty slow compared to other ways of iteration. I also think it's a bit odd that your animation framework relies on a utility lib for low level iteration.

from lodash.

jeremyckahn commented on April 27, 2024

@jdalton I agree that animation frameworks should fast, and performance optimization is certainly a high-priority goal of Rekapi. That being said, I don't know how else to compute animation state without inspecting each property and calculating the state from that.

As for relying on a utility library, it makes Rekapi's code far more concise and lets me avoid reinventing the wheel for many common functions. The performance has been great so far. Over-optimization leads to unmaintainable code.

from lodash.

jdalton commented on April 27, 2024

Patched here.

from lodash.

jdalton commented on April 27, 2024

That being said, I don't know how else to compute animation state without inspecting each property and calculating the state from that.

State data (like coordinates or color) can be stored in objects but lists like _actors and _canvasActors would fit better as arrays.

As for relying on a utility library, it makes Rekapi's code far more concise and lets me avoid reinventing the wheel for many common functions. The performance has been great so far. Over-optimization leads to unmaintainable code.

Ya I get that. As you've gotten a bit of a perf boost by replacing Underscore with Lo-Dash you might take it a step further in perf critical areas and rely on your own custom iteration code that is tailored more for your specific needs (couldn't hurt to look anyways).

from lodash.

jdalton commented on April 27, 2024

As of Lo-Dash v0.8.2 _.max, _.min, and _.shuffle work with objects.

from lodash.

lock commented on April 27, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

from lodash.

Moving sortBy into the collections category about lodash HOT 23 CLOSED

Comments (23)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent