Content bug: Number.isInteger polyfill slightly incorrect and a bit confusing

[tjcrowder]

What page(s) did you find the problem on?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger

Specific page section or heading?

Polyfill

What is the problem?

1. It creates the function with enumerable: true (because it uses assignment to create it); it should be enumerable: false
2. It seems incorrect because the algorithm says floor(abs(value)) === value, but the polyfill only does Math.floor(value) === value (leaving out the abs operation). One could naively jump to the conclusion it wouldn't work correctly for negative numbers (I did, oops 😃), but it does seem to work, probably because it doesn't really matter whether you floor or ceil (or trunc) the value, all we really need to know is that doing that does or doesn't lose some fractional portion. Still, I assume the algorithm is written the way it is for a reason, it's probably best if the polyfill implements it faithfully.
3. The function ends up with no name, rather than the name isInteger (which is spec'd).

What did you expect to see?

Something like this:

if (!Number.isInteger) {
    Object.defineProperty(Number, "isInteger", {
        value: function isInteger(num) {
            const absnum = Math.abs(num);
            return typeof num === "number" &&
                isFinite(num) &&
                Math.floor(absnum) === absnum;
        },
        writable: true,
        configurable: true,
    });
}

Or alternatively, that without the Math.abs operation and an explanation why it's not needed although it's specified.

Happy to do a PR with the version above.

Did you test this? If so, how?

Yes, by:

1. Looking at the resulting object descriptor if the polyfill is applied
2. Testing whether negative numbers seemed to work correctly (they do in both the original and the update)
3. Looking for a name on the function if the polyfill is applied

(1) and (3) are incorrect for the current polyfill, correct for the one above.

[teoli2003]

Given that all browsers support this feature for years (except IE). Shouldn't we delete the polyfill?

[tjcrowder]

@teoli2003 That works too. 😃 Although (heaven help us) there are still enterprises reliant on IE11 because of legacy junk. :-| I had to do some maintenance for one of those just earlier this year. (I did tell them they should move off it, or at least use an AD policy to disallow using it for browsing internet sites, restricting it to the intranet apps they still rely on.)

[teoli2003]

That's true.

I see there is a polyfill link in the See also section? Is the content of that link pertinent? If so, we could keep it, still helping for IE11 without having the burden to maintain it ourselves.

[tjcrowder]

@teoli2003 It depends entirely on what position MDN has or wants to have with regard to polyfills: Have them, or defer to https://github.com/zloirock/core-js or https://github.com/es-shims or other such. That's probably an editorial-level decision. Having a polyfill on the page does provide educational value, but at the cost of maintenance (this specific one is unlikely to change, but others may over time as the spec changes).

[teoli2003]

We are leaning towards linking to external websites. My question was more: Is the linked polyfill good?

[tjcrowder]

It doesn't follow the letter of the spec, and is extremely hard to find in the code base (it's here), so it has zero educational value. I assume it works and is added correctly (e.g., with the right value for enumerable) but that's an assumption. I didn't dig deep enough to double-check that.

[wbamberg]

It depends entirely on what position MDN has or wants to have with regard to polyfills: Have them, or defer to https://github.com/zloirock/core-js or https://github.com/es-shims or other such. That's probably an editorial-level decision.

Yes, I agree. The original discussion of this was https://discourse.mozilla.org/t/mdn-rfc-001-mdn-wiki-pages-shouldnt-be-a-distributor-of-polyfills/24500 - I think whatever we do decide to do about documenting polyfills on MDN, there's a pretty clear consensus for not maintaining them in content. That's also the starting premise of openwebdocs/project#27 .