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.

Sign up for GitHub

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

Jump to bottom

react-i18n - add from-dictionary-index plugin mode #1197

Merged
merged 1 commit into from
Nov 29, 2019
Merged

react-i18n - add from-dictionary-index plugin mode #1197

merged 1 commit into from
Nov 29, 2019

Conversation

GoodForOneFare
Copy link
Member

@GoodForOneFare GoodForOneFare commented Nov 27, 2019 •
edited
Loading

Description

Adds a from-dictionary-index plugin. With this, folks with lots of CPU (or time) can generate multiple versions of their app. Each with a specific locale embedded directly into its JS. The advantages of this are:

  • Bundlers like webpack don't get bloated with metadata about where to find async translations
  • Users don't have to download multiple sets of translations

🎩

  • Check out the solo-i18n-test branch of web-rails-proving-ground
  • Generate French index files using:
    • node -e "require('@shopify/react-i18n/generate-dictionaries').generateTranslationDictionaries(['fr'])"
  • Clear out old transpilation artifacts
    • rm -fr tmp/sewing-kit/cache/webpack
  • dev run
  • Ensure French translations appear in the customers route
  • Ensure only French translations are embedded in JS files

Customers 2019-11-27 15-09-36

Type of change

  • `@shopify/react-i18n Minor: New feature

Checklist

  • I have added a changelog entry, prefixed by the type of change noted above

@GoodForOneFare GoodForOneFare force-pushed the react-i18n-generated-dictionary-plugin branch 2 times, most recently from 832c796 to 947257b Compare November 27, 2019 19:29
@GoodForOneFare GoodForOneFare changed the title WIP - generated dictionary plugin react-i18n - add from-dictionary-index plugin mode Nov 27, 2019
@GoodForOneFare GoodForOneFare force-pushed the react-i18n-generated-dictionary-plugin branch 4 times, most recently from e23d529 to 4df7856 Compare November 27, 2019 19:39
@GoodForOneFare GoodForOneFare marked this pull request as ready for review November 27, 2019 20:10
@GoodForOneFare GoodForOneFare force-pushed the react-i18n-generated-dictionary-plugin branch from 4df7856 to 2f55579 Compare November 27, 2019 20:16
GoodForOneFare
GoodForOneFare commented Nov 27, 2019
View reviewed changes
.travis.yml
@@ -15,7 +15,7 @@ matrix:
- <<: *node_container
name: 'Node - build, type-check, and end-to-end tests'
script:
- yarn build
- yarn build --verbose
Copy link
Member Author

Choose a reason for hiding this comment

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

Travis keeps timing out because the build takes >10m with no output. Making it verbose works around Travis' behaviour 🤷🏻‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

tbh it also works around my behaviour, I get a gut feeling that it's broken every time since we changed it from the old setup :P

@GoodForOneFare GoodForOneFare mentioned this pull request Nov 28, 2019
Merged
2 tasks
cartogram
cartogram reviewed Nov 28, 2019
View reviewed changes
packages/react-i18n/README.md Outdated Show resolved Hide resolved
packages/react-i18n/README.md Outdated Show resolved Hide resolved
packages/react-i18n/src/babel-plugin/test/babel-plugin.test.ts Outdated Show resolved Hide resolved
marutypes
marutypes approved these changes Nov 28, 2019
View reviewed changes
Copy link
Contributor

@marutypes marutypes left a comment

Choose a reason for hiding this comment

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

Some small comments but this looks rad

packages/react-i18n/README.md Show resolved Hide resolved
packages/react-i18n/src/babel-plugin/generate-dictionaries.ts
);
}

const translationBuckets = findTranslationBuckets(rootDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a pretty great mental image

packages/react-i18n/src/babel-plugin/generate-dictionaries.ts Show resolved Hide resolved
packages/react-i18n/src/babel-plugin/shared.ts Show resolved Hide resolved
.travis.yml
@@ -15,7 +15,7 @@ matrix:
- <<: *node_container
name: 'Node - build, type-check, and end-to-end tests'
script:
- yarn build
- yarn build --verbose
Copy link
Contributor

Choose a reason for hiding this comment

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

tbh it also works around my behaviour, I get a gut feeling that it's broken every time since we changed it from the old setup :P

yarn.lock Show resolved Hide resolved
packages/react-i18n/src/babel-plugin/generate-dictionaries.ts
translationFilePaths,
);
const acc = await accPromise;
acc[locale] = merge(
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we have to use lodash merge here because this needs to merge deeply and whatnot? Does make me wonder if we want to use groupBy from lodash for our bucketing operations to make them clearer, but /shrug

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmmm, I'm conflicted about this because I hate adding lodash to anything 😸 The merge/clone things were unavoidable, but in for a penny...

I've created an issue to track this enhancement: #1200

@GoodForOneFare GoodForOneFare force-pushed the react-i18n-generated-dictionary-plugin branch from f035f33 to c97c80c Compare November 29, 2019 06:37
@GoodForOneFare GoodForOneFare mentioned this pull request Nov 29, 2019
Closed
@GoodForOneFare GoodForOneFare force-pushed the react-i18n-generated-dictionary-plugin branch from c97c80c to a8f418f Compare November 29, 2019 07:05
@GoodForOneFare GoodForOneFare merged commit 38e819a into master Nov 29, 2019
@GoodForOneFare GoodForOneFare deleted the react-i18n-generated-dictionary-plugin branch November 29, 2019 14:32
@GoodForOneFare GoodForOneFare temporarily deployed to production November 29, 2019 15:58 Inactive
@GoodForOneFare GoodForOneFare mentioned this pull request Nov 29, 2019
Closed
1 task
@theodoretan theodoretan temporarily deployed to gem December 10, 2019 21:19 Inactive
@michenly michenly mentioned this pull request Dec 13, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cartogram cartogram cartogram left review comments

@marutypes marutypes marutypes approved these changes

Assignees
No one assigned
Labels
Package: react-i18n Type: Enhancement 📈 Enhancement to our codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GoodForOneFare @cartogram @marutypes @theodoretan