search

i18next / i18next-browser-languageDetector

language detector used in browser environment for i18next
MIT License
865 stars 88 forks source link

chore: set browsers target to `defaults` #286

Closed VIKTORVAV99 closed 5 months ago

VIKTORVAV99 commented 5 months ago

Technically a breaking change but I believe it makes sense to only support es6 supported browsers which get's rid of a lot of the polyfills that babel need to insert which is inflating the bundle a lot. It also allows babel to use object deconstruction internally in the transformers and other newer syntax so further reduce the bundle.

The minified file in the top level of the repo went from 7 540 byte to 6 341 byte. A reduction of ~16%.

ES6 is supported by 97.1% of the browsers right now: https://browsersl.ist/#q=%22browserslist%22%3A+%5B%0A++++++++%22fully+supports+es6%22%0A++++++%5D

I didn't see any reference to which browsers or JS versions that are supported so I don't believe this requires any documentation updates but please let me know if it does.

PS: If you prefer this as a option inside the .babelrc file instead I'd be happy to change it but in general I think it's best to set it in the package.json so all tooling picks it up.

Checklist

Checklist (for documentation change)

VIKTORVAV99 commented 5 months ago

I would also be happy to change this to what matches the main i18n package but that requires installing @babel/preset-react as well. (Which shouldn’t be needed in either btw as there is no jsx in the source code.)

Might make sense to have them all unified though.

adrai commented 5 months ago

should be in line with react-i18next

adrai commented 5 months ago

at the end it's: "1kb of size reduction" vs. "dropping support for a lot of older environments"... is this really a huge benefit?

VIKTORVAV99 commented 5 months ago

at the end it's: "1kb of size reduction" vs. "dropping support for a lot of older environments"... is this really a huge benefit?

Considering we have to load i18next and all plugins for it in our index bundle making that as small as possible is very much something that would benefit us.

There is also a small environmental impact to this as it would save just us tens of gigabytes in lowered data transfers per year from just this simple change.

I haven't checked how it would look with the react-i18next config in terms of numbers but I'd say dropping support for 2.9% of the users with this current config is not really a huge deal. Especially since the consumers of this package don't have to update if they really need it. For example 100% of our own user base at @electricitymaps would be supported and there would only be upsides to this for us.

adrai commented 5 months ago

i think it's relatively simple... we can update it... but if there will be complaints... we will consider to revert it

VIKTORVAV99 commented 5 months ago

I'll update this PR to match the react-i18next config as soon as I have some time then.

VIKTORVAV99 commented 5 months ago

I have matched the react-i18next config where it made sense, @babel/plugin-transform-runtime is already included in the @babel/preset-env and @babel/preset-react is not needed as this package don't have any jsx source code.

The size of the generated minified code is now 6 370 bytes. Likely cause it is using const and let as the source code did and not converting everything to var.

VIKTORVAV99 commented 5 months ago

BTW: Once this is merged I plan on opening a follow up PR that changes code to take advantage of these changes.

jasperfirecai2 commented 2 months ago

do note that supports es6 and defaults are a different audience reach.