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

Use single webpack build + embed only used locales in the HTML file #2369

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Apr 9, 2020

Changes

  • Use a single Webpack build instead of per-locale which significantly decreases building time, but almost doubles the resulting bundle.js size - resolves speed up multi-language build #2342
  • Don't import locales in helpers/translation.js which clears up bundle.js of all the locales data (reducing back its size) but doesn't provide the locales info in the JS
  • Instead, let server.js and generate-index.js embed max 2 locales in the rendered HTML files (the current locale + english as a fallback), slightly increasing their size (still keeping them < 100kB)

Tested

  • Ran ./mobileapp/generate-index.js and verified that the HTMLs get generated
  • docker-compose build && docker-compose up works also with changing the languages
  • docker-compose run ... web npm run watch && yarn start works also with changing the languages

Checklist

Ideas for the Future

  • Instead of embedding locales in the HTML, it would be great if we coud load them async from the frontend - implementing this at the moment (before fully transitioning to React) would be somewhat dirty and perhaps not worth the effort, but it would be cool if we could eventually:
    • Manage locale as a variable in the Redux state instead of the Express server
    • Switch between languages without having to reload the whole page
    • Keep our HTMLs neat and clean

@fbarl fbarl requested a review from corradio April 9, 2020 17:17
@fbarl fbarl self-assigned this Apr 9, 2020
Copy link
Member

@corradio corradio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!

@fbarl fbarl force-pushed the fbarl/2342-dont-bundle-locales branch from fc52be9 to fbbc1f8 Compare April 14, 2020 17:34
@fbarl
Copy link
Contributor Author

fbarl commented Apr 14, 2020

@corradio Since all our locales data is static and it goes through the PR review process (+ we already use dangerouslySetInnerHTML elsewhere in the code), I won't tackle the potential XSS vulnerability issue here.

However if you think that's still something we should address soon, I'll open a separate issue for it and merge this PR as is.

@fbarl fbarl merged commit 1bd0d97 into master Apr 14, 2020
@fbarl fbarl deleted the fbarl/2342-dont-bundle-locales branch April 14, 2020 17:56
@corradio
Copy link
Member

corradio commented Apr 15, 2020 via email

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.

speed up multi-language build
2 participants