paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 387 forks source link

Bug in roundHandlesBoundaryConditions #432

Open LMLB opened 7 years ago

LMLB commented 7 years ago

In roundHandlesBoundaryConditions, I think the the -0.5 should be positive in the second half (i.e. Math.round(-0.5 + (Number.EPSILON / 3.99)) === 1).

LMLB commented 7 years ago

In any case, the code currently replaces Math.round if Math.round(-0.4999...) isn't equal to 1.

ljharb commented 7 years ago

Can you provide a test case that you think should pass, that fails with our current code?

Indeed I see Math.round(-0.5 + (Number.EPSILON / 3.99)) equalling -0 and Math.round(0.5 + (Number.EPSILON / 3.99)) equalling 1 in most browsers.

Per https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L2071-L2072, I checked Safari 8, IE 11, and Opera 12, and got the same results.

git spelunking reveals https://github.com/paulmillr/es6-shim/commit/55dc1fa1e58753b3ee1a4162bbc60b66cb47099bhttps://github.com/paulmillr/es6-shim/commit/a20e99fe5876f055b7455dd63a65b3cdc3122546https://github.com/paulmillr/es6-shim/commit/90c803f68390dd13fd5297b1e2d54d44f8dac94b, in which I reference https://github.com/kangax/compat-table/issues/392#issuecomment-70381406, in which @Yaffle indicated this is how the test should go.

@Yaffle, could you confirm that this is the right fix here?

Yaffle commented 7 years ago

The polyfill replaces the native Math.round even if it is good. The right fix is to remove the second equals operator:

-var roundHandlesBoundaryConditions = Math.round(0.5 - (Number.EPSILON / 4)) === 0 &&
-    Math.round(-0.5 + (Number.EPSILON / 3.99)) === 1;
+var roundHandlesBoundaryConditions = Math.round(0.5 - (Number.EPSILON / 4)) === 0;

It seems it was added by mistake.

ljharb commented 7 years ago

There was definitely a reason that I added the second condition, but if we can't reproduce what it was, then it's probably worth removing it. I'll do so when I have time to rerun tests on all the browsers and versions.

Yaffle commented 7 years ago

@ljharb which tests do you need to rerun? Can I help you?