Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only load Intl data for current language #3130

Merged
merged 3 commits into from May 22, 2017

Conversation

nolanlawson
Copy link
Contributor

@nolanlawson nolanlawson commented May 19, 2017

This reduces the size of application.js from 1006697 bytes (1.01 MB) to 662157 (662 kB), while slightly increasing common.js from 320982 bytes (321 kB) to 337157 (337 kB) (fixed, see below).

With this change, all locale data is moved into a separate pack for each language, and then that language's JS pack is loaded after common.js. These packs are between 12K and 20K, so for a language like English the overall JS payload for the app page drops from 1327679 bytes (1.33 MB) to 1008757 bytes (1.01 MB), so in total 319 kB are removed 1000032 bytes (1 MB), so in total 328 kB are removed.

Note: I noticed that react-intl doesn't have any data for the io language (Ido, a constructed language derived from Esperanto), so I just used en, which is what react-intl apparently does anyway when it can't find the locale data. I also added a TODO in case anybody wants to fix it.

This commit is also live on https://malfunctioning.technology in case you want to test it out; I've confirmed that it works on the landing page (logged in and not logged in), app page, and settings page. I've also confirmed that I can switch between English, French, Ido, and Brazilian Portuguese.

@nolanlawson
Copy link
Contributor Author

Before and after (click to expand):

JS files before:
492K	0-1af6e0d69c0073d1a436.js
88K	1-ddc4d03c660769f976ed.js
984K	application-4cc778a2892b9ed4f620.js
316K	common-1bbde5d781d138ebc7a4.js
16K	public-d11fe12000ba9f1f4f04.js
JS files after:
492K	/tmp/packs-intl/0-5d74313323661eae82e1.js
88K	/tmp/packs-intl/1-dfecd30addb0f9a369fd.js
648K	/tmp/packs-intl/application-62acb774bf6eb8dcb769.js
332K	/tmp/packs-intl/common-c186c1a21f9a10f63b51.js
20K	/tmp/packs-intl/locale_ar-89da1010b74919105fbe.js
12K	/tmp/packs-intl/locale_bg-16796469bcb3be33c66d.js
16K	/tmp/packs-intl/locale_ca-7d7eace8e78574b97127.js
12K	/tmp/packs-intl/locale_de-aecabddd8e961234fa26.js
12K	/tmp/packs-intl/locale_en-8f4ea55a5dd344bfc495.js
12K	/tmp/packs-intl/locale_eo-15817e28895f6feab48d.js
24K	/tmp/packs-intl/locale_es-27fd0c2601af8e3585c9.js
16K	/tmp/packs-intl/locale_fa-40d3f0aa4e1e5c6bf613.js
12K	/tmp/packs-intl/locale_fi-b461615101b2c487d15b.js
16K	/tmp/packs-intl/locale_fr-0f862a5d61d7a8103369.js
16K	/tmp/packs-intl/locale_he-8499fdb4d26985c199fc.js
12K	/tmp/packs-intl/locale_hr-8649ea9db715bf124563.js
12K	/tmp/packs-intl/locale_hu-3ab1209bbc578a51779c.js
12K	/tmp/packs-intl/locale_id-de8a532535982e430e23.js
12K	/tmp/packs-intl/locale_io-43223382784620cfeaad.js
12K	/tmp/packs-intl/locale_it-a4793e6f31b34960fe58.js
12K	/tmp/packs-intl/locale_ja-cfd520dc60a25e64a53b.js
12K	/tmp/packs-intl/locale_nl-ed5ef64753773f332036.js
12K	/tmp/packs-intl/locale_no-bdfaaeab3cba995f738d.js
12K	/tmp/packs-intl/locale_oc-c2131ee0ae7d1abaafe5.js
12K	/tmp/packs-intl/locale_pt-BR-2135ec141dfba4edfc4b.js
12K	/tmp/packs-intl/locale_pt-e4ff6234abe03cca8617.js
20K	/tmp/packs-intl/locale_ru-edf43a406e3a5a3d4674.js
12K	/tmp/packs-intl/locale_tr-f1b39c66be454914a120.js
20K	/tmp/packs-intl/locale_uk-50cbc4c03edc516e278b.js
12K	/tmp/packs-intl/locale_zh-CN-da0334aa26f7ded7afc9.js
12K	/tmp/packs-intl/locale_zh-HK-662384a4c2eaec1d545b.js
16K	/tmp/packs-intl/public-bf9a460760c231a33228.js

@nolanlawson
Copy link
Contributor Author

What's the deal with these codeclimate rule violations like Definition for rule 'jsx-a11y/no-noninteractive-element-interactions' was not found? Not sure what I should do here. 😕 Running npm run test:lint doesn't repro…

@nolanlawson
Copy link
Contributor Author

nolanlawson commented May 19, 2017

Webpack Bundle Analyzer view

Interestingly, it looks like en, pt, and zh are being loaded in common.js because they are "common" (i.e. there's pt-BR+pt, zh-CN+zh-HK, etc.). That's not ideal, but it can be fixed in a separate PR.

@nolanlawson
Copy link
Contributor Author

OK, fixed the issue by specifying that common.js should only be derived from application.js and public.js. So now common.js is back down to its original size. Here's the updated Webpack Bundle Analyzer view and updated files:

Files list
492K	0-eb73491af949d70bfb27.js
88K	1-dfecd30addb0f9a369fd.js
648K	application-d7ab6f77e8fdde2a0be4.js
316K	common-a69546f576ca045017b8.js
20K	locale_ar-78acd295dbb160da465f.js
12K	locale_bg-6d8fbbd748848c318747.js
16K	locale_ca-4edd85c564c2a19db372.js
12K	locale_de-46c5c75f8d6ccd12ffeb.js
20K	locale_en-5b14091da9869f753b3c.js
12K	locale_eo-6b2b6c3179f95e133174.js
24K	locale_es-a2c17986d3935797dd37.js
16K	locale_fa-0eca47ae533f9cf380d2.js
12K	locale_fi-297b86b26c912e709b0c.js
16K	locale_fr-b33f5542f4e19f0380cb.js
16K	locale_he-b5a350e06410831052bb.js
12K	locale_hr-2925b98418be40c4ada4.js
12K	locale_hu-08da4161e74336a78932.js
12K	locale_id-17dc15fcf40983b7b4c0.js
20K	locale_io-e531d261eb13d58833c9.js
12K	locale_it-7d25ec4e346a4fa88de4.js
16K	locale_ja-0d289b5ae65dc26ab9cd.js
12K	locale_nl-c67232c49a7daae752d0.js
12K	locale_no-0b8f21af49febb3bb66b.js
12K	locale_oc-0112a7ffec3ed4c2dbf9.js
16K	locale_pt-26e815fb90810213f512.js
16K	locale_pt-BR-a40d6d5a808a4a319154.js
20K	locale_ru-9250440debedb8fa3a75.js
16K	locale_tr-1017aeeb26da463d8530.js
20K	locale_uk-0ed1ec9362410c9a93e8.js
16K	locale_zh-CN-55fdd0756d415757c948.js
16K	locale_zh-HK-460b9170bae7e00c39eb.js
16K	public-5951a35790f2f47b6780.js

@ykzts
Copy link
Sponsor Member

ykzts commented May 19, 2017

@nolanlawson

What's the deal with these codeclimate rule violations like Definition for rule 'jsx-a11y/no-noninteractive-element-interactions' was not found? Not sure what I should do here. 😕 Running npm run test:lint doesn't repro…

It seems that it was fixed by #3131

@Gargron
Copy link
Member

Gargron commented May 19, 2017

I expected this to use require.context or System.import like the polyfills are loaded before main, I'm not too eager for all of these entrypoint packs, it's a lot of boilerplate and extra complexity for translators of new languages...

@nolanlawson
Copy link
Contributor Author

nolanlawson commented May 19, 2017

I considered doing this as a dynamic import, but the problem with that approach is that it's an extra roundtrip to load the language file. (I.e., wait for JS to load on the client side, check the locale from data-props, go fetch that language file, wait for that JS to load, then run main().) It seemed like a shame given we already know the locale on the server-side.

This isn't any more work for new translators AFAICT; you just need to add a 3-line file under packs/, as opposed to adding an import for the react-intl locale in containers/mastodon.js plus adding an import to locales/index.js (which are both now just moved to that packs file).

@nolanlawson
Copy link
Contributor Author

Tested on a Nexus 5 running Chrome 58 throttled to 3G (cache disabled) to see the improvement from this PR. Looks like it improved DOMContentLoaded from 5.83s to 5.28s, so ~0.5s faster.

Before:

screenshot 2017-05-19 18 59 09

After:

screenshot 2017-05-19 19 04 10

@nolanlawson
Copy link
Contributor Author

nolanlawson commented May 22, 2017

To make this PR a bit more palatable and respond to @Gargron's feedback, I've updated with the following changes:

  1. All locale_*.js packs are automatically generated and written to tmp/
  2. A normal module is used to store the current locale rather than the window object

Performance-wise this is exactly the same as before, but now future translators have even less work they need to do, because the boilerplate is auto-generated. Just create <locale>.json and you're done.

How it works: locale data is automatically located from react-intl/locale-data/, falling back to mastodon/locales/locale-data/ (e.g. for Occitan), and then back to react-intl/locale-data/en (e.g. for Ido) as necessary. Also, the conversion for locales like zh-TW and pt-BR is automatically done, and unnecessary "common" modules (e.g. common between zh-TW and zh-CN) are not loaded into common.js. Here's an updated Webpack Bundle Analyzer view you can inspect.

@Gargron Gargron merged commit 9d04de1 into mastodon:master May 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants