Remove string trim polyfill

[adamnoakes]

This library is actually quite large (21.6kb https://bundlephobia.com/result?p=node-polyglot@2.4.0). This is due to the String.prototype.trim polyfill, and it's dependencies (mainly es-abstract).

String.prototype.trim has been supported by all major browsers for nearly 10 years https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trim so it's probably not worth including in the library.

By removing it, it should save just over 15kb of minified JS (according to bundlephobia).

Dependencies before: https://bundlephobia.com/scan-results?packages=for-each@0.3.3,has@1.0.3,string.prototype.trim@1.1.2,warning@4.0.3

Dependencies after: https://bundlephobia.com/scan-results?packages=for-each@0.3.3,has@1.0.3,warning@4.0.3

[adamnoakes]

@ljharb agreed it would be a break change, but one i think is worthwhile. I can add a note to the readme with browser support and then the consumer of library can choose to include their own polyfill should they need to.

[adamnoakes]

@ljharb If ES3 support by this library is a must, then an option would be to define the method in the library which would be significantly smaller than the current polyfil e.g. based off https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trim#Polyfill

function trim(str) {
    if (String.prototype.trim) {
        return str.trim();
     }
    return str.replace(/^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g, '');
}

[ljharb]

Like all MDN polyfills, that's actually incorrect and doesn't reflect proper whitespace; the one in the string.prototype.trim package is the minimum that is required.

[adamnoakes]

It may not be perfect, but it may be OK for most use cases which really comes back to my previous point that it should be down to the consumer of the library to include a polyfill should they need it.

[ljharb]

I see it as the reverse: if you know you don't need it, it's trivial for you to substitute require('string.prototype.trim') in your bundler for Function.call.bind(String.prototype.trim). The default case should work everywhere, no polyfills needed.

[adamnoakes]

I don't disagree that the library should be standalone but there becomes a point when it doesn't make sense to support browsers of a certain age. Can you imagine if every library included polyfills for everything, you might end up with an application that works nowhere, because it's so bloated.

Doing some digging around it would appear that airbnb doesn't even support < IE9 anymore https://github.com/airbnb/babel-preset-airbnb/blob/master/index.js https://www.airbnb.co.uk/help/article/446/which-internet-browsers-work-best-on-airbnb

And sure, i can work around it in my bundler, but that doesn't remove the dead code for the other x amount of people using the library.

[ljharb]

There's been unicode whitespace changes since ES5, so the package is actually needed in much more recent browsers than IE 9, especially in a library that's meant to handle multiple languages. It would be unfortunate if the whitespace classification changes of the mongolian vowel separator broke Mongolian translations for someone in the name of dropping old IE :-p

[adamnoakes]

I'll be honest, i've never heard of a mongolian vowel separator. But looking at the polyfil, it doesn't appear to check for that before using the native implementation so i'm unsure if it's going to solve that problem.

[ljharb]

That's also fair, and is a likely bug in the polyfill package. I've filed https://github.com/es-shims/String.prototype.trim/issues/6 to track it.

[adamnoakes]

I noticed es-shims/String.prototype.trim#6 is closed. Is there still a need for the polyfil in this library?

[ljharb]

the need remains; supporting browsers that don’t have the functionality, old tho they may be.

I’m going to close this PR; you can (and should) configure your bundler to alias out packages that robustly provide functionality that your supported browsers already have natively (of which there are many beyond the string trim package).

[adamnoakes]

I don't think i'm going to change your mind, but i ask you to at least consider the advice on https://w3ctag.github.io/polyfills/#consider-whether-to-include-polyfills notably "How large is the polyfill code relative to your library code?".

The polyfil increases this library by about 3 times and the browser it's there for is very old and not even supported by yourselves.

I think https://w3ctag.github.io/polyfills/#consider-alternatives point 1 is the best option. And is the option many library owners are taking e.g. https://reactjs.org/docs/javascript-environment-requirements.html