Wrong value for _.isEqual(0, new Number(Number.MIN_VALUE))

[dubzzz]

The current implementation of underscore is returning an invalid answer for:

_.isEqual(0, new Number(Number.MIN_VALUE))
// expected to be false but was true before the fix

Similar to a bug found in Jest https://github.com/facebook/jest/issues/7941

---

This bug has been discovered with the following test - property based testing test:

import * as _ from "underscore";
import * as fc from "fast-check";

describe("_.toEqual", () => {
  it("should be symmetric", () => {
    fc.assert(
      fc.property(
        fc.anything({ withBoxedValues: true }),
        fc.anything({ withBoxedValues: true }),
        (a, b) => _.isEqual(a, b) === _.isEqual(b, a)
      ),
      { numRuns: 1000000 }
    );
  });
});

Please note that such kind of tests are currently used within Jest to avoid future issues.

[jashkenas]

I’d be happy to defer to whichever version you think is best for this, because I don't have a particularly strong opinion about how this should behave, in the context of JavaScript floating point ubiquity.

But if we do decide to make a change here, it would be a breaking change.

[dubzzz]

@jashkenas IMO as the original source code was considering 0 to be different from -0, I'd say the change should consider 0 to be different from Number.MIN_VALUE.

What's your opinion?

If it can help for the decision both Jasmine and Jest opted to distinguish 0 from Number.MN_VALUE, see the fixes https://github.com/facebook/jest/pull/7948 and https://github.com/jasmine/jasmine/pull/1764

[jgonggrijp]

I made a new comment above that numbers four options from most to least fuzzy. The current code behaves like option (2) or (3) depending on the values of the operands and the order in which they are fed into _.isEqual. So any change that makes this consistent is a breaking change, but I think you can still share this under "bug fixes" if you choose option (2) or (3).

If choosing option (1) or (4), maybe it should also be considered a semantic change. Option (4) is what @dubzzz is currently proposing.

Edit to add: option (4) is probably the most EGAL, so I'm not ruling out that this is what was originally intended, but this is not how the code on the master branch actually behaves.

[jashkenas]

Thanks for laying out the tl;dr of the four options. It sounds like it might be time to have "the big semver conversation" if we go with option 4...

[jgonggrijp]

I think I agree with @dubzzz that option 4 is the most correct, but it is not a strong opinion.

As for semver, we could work with parallel 1.x and 2.x branches for a while so we don't have to rush the 2.0 release. One of those branches could be master.

I'm going to compose an email to you because there's something I'd like to bring to the table for a potential 2.0.