Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

Uplift i18n code to use dynamic imports #1720

Closed
bjankord opened this issue Jul 20, 2018 · 5 comments
Closed

Uplift i18n code to use dynamic imports #1720

bjankord opened this issue Jul 20, 2018 · 5 comments

Comments

@bjankord
Copy link
Contributor

Feature Request

Description

Currently, we use require.ensure to dynamically load i18n data based on what the current locale is set as. This is based on the recommendation from the Intl polyfill we used. This has worked well, however, webpack recommends using dynamic imports over require.ensure. There is a possibility require.ensure becomes deprecated, however that doesn't seem likely yet.
This issue is to start discussion on the update.

@bjankord
Copy link
Contributor Author

bjankord commented Jul 20, 2018

// Dynamic import for translations
const loadArTranslations = (callback, scope) => {
  import( /* webpackChunkName: "ar-translations" */'./ar.js')
    .then(module => { callback.call(scope, module);})
    .catch(error => { console.log(error) });
};
// Dynamic import for intl data
const loadArIntl = () =>
  import( /* webpackChunkName: "ar-intl-local" */'intl/locale-data/jsonp/ar.js')
    .then(module => {})
    .catch(error => { console.log(error) });
// Dynamic import for intl polyfill
import( /* webpackChunkName: "intl-polyfill" */'intl')
    .then(module => {
      loadIntl(locale);
      loadTranslations(locale, callback, scope);
    })
    .catch(error => { console.log(error) });

@bjankord bjankord added this to the Backlog milestone Jul 26, 2018
@bjankord
Copy link
Contributor Author

bjankord commented Sep 26, 2018

They've added support for dynamic imports in node_modules, in create-react-app v2. create-react-app explicitly prevents usage of require.ensure. Need to test and see how terra-ui components works with this change.

Edit: I've explored this some and I was able to get a variation of terra-base / terra-i18n code to work with create-react-app, but had to modify some code.

The aggregated translations pre-build tool would need to support outputting translation and intl loader files that use dynamic imports.

In terra-core, we use require.ensure in defaultIntlLoaders.js, defaultTranslationsLoaders.js, and the i18nLoader.js files.

These instances of require.ensure can all be flipped over to dynamic imports, however during the babel compilation / transformation process for code we distrubute in lib, the dynamic import code will be output as require.ensure instead of the dynamic import syntax so these files in terra-core will not work in create-react-app, they would work for our Cerner app starter kits.

In create-react-app, we wouldn't need to use the defaultIntlLoader.js or defaultTranslationsLoader.js files and instead, would use the ones generated via the aggregated-translations pre-build tool, so that part can be solved/mitigated.

This leaves the require.ensure statement in i18nLoader.js to figure out how to adapt if we want to use terra-base / terra-i18n in create-react-app.

At first, I thought it would be challenging to get terra-base/terra-i18n (code-splitting for translations) working in CRA v2 without ejecting, but it seems possible, if we lift the i18nLoader function from terra-i18n/src/i18nLoader.js into terra-base. This would allow people to import Base from 'terra-base/src/Base'; (would need to avoid using JSX in this file) which would use dynamic import code instead of the transpiled require.ensure syntax.

Until then, it may be easier to show users that want to use CRA v2 how to set up react-intl's <IntlProvider /> and load in a single aggregated translation file + associated intl data.

@noahbenham
Copy link
Contributor

noahbenham commented Oct 4, 2018

Leaving this here in case anyone has issues with Create React App 2.

Discussed with @bjankord offline - CRA2 consumers will also need to add dir="ltr to the html tag of public/index.html. Without it, this type of behavior could be seen:

screen shot 2018-10-04 at 12 14 32 pm screen shot 2018-10-04 at 12 14 40 pm

@bjankord
Copy link
Contributor Author

bjankord commented Mar 19, 2019

With the work for #1941, we'll be removing defaultIntlLoaders.js, defaultTranslationsLoaders.js so that will resolve the require.ensure syntax in those files. We've also updated terra-toolkit to allow aggregate-translation files to be generated with dynamic imports. That just leaves the require.ensure syntax in i18nLoader.js. This code can be reworked so it handles the Intl check in a separate file and exports either the intl polyfill on window.Intl based on intl support removing the need for require.ensure.

@bjankord
Copy link
Contributor Author

This has been resolved in [email protected]/[email protected]

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants