Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: support date objects and other non-primitive values #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dominicbarnes
Copy link

@dominicbarnes dominicbarnes commented Jul 1, 2018

When sorting by multiple props that include Date objects, you'll end up with incorrect sorting in cases where the underlying date value is the same. The use of === (after checking < and >) will always return false for Date objects, since === compares identity for objects. The result of the final result * sortOrder becomes NaN, which then causes iteration to stop (since the check is looking for === 0).

Just to be clear, this edge-case does not occur when you are only sorting by 1 property, it only comes up for properties following the aforementioned Date objects.

The change here assumes that the returned value will be 0, and will only change to -1 for < and 1 for >. This change is backwards-compatible, and will allow non-primitive values (such as Date) to be handled more gracefully.

This change also includes a test case that was failing before my change, but now passes.

@dominicbarnes dominicbarnes force-pushed the fix-dates branch 3 times, most recently from 39b1db7 to ebbf1c8 Compare July 1, 2018 03:20
@dominicbarnes
Copy link
Author

dominicbarnes commented Jul 1, 2018

As an aside, my current workaround is to provide my own custom mapper that looks like this:

function sortMapper(key, value) {
  return value.valueOf()
}

My first proposal was actually going to be to use valueOf() before checking <, > and ===. This actually seemed to work pretty well, but it would have broken backwards-compatibility. This is because anything created with Object.create(null) would throw an exception and anything with a prototype that provided a custom valueOf() would have had their behavior altered. The above change seems to avoid all of that while still getting the job done.

dominicbarnes added a commit to dominicbarnes/pokemongo-tools that referenced this pull request Jul 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant