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

Replace client/languages with packages/languages #46709

Merged
merged 6 commits into from
Nov 8, 2020
Merged

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Oct 23, 2020

Language data currently held in client/languages is required outside the calypso scope.

We need this data to enable moving the localizeUrl function outside of calypso (I'm looking to move this function into i18n-calypso.)

For examples, linking to support documentation in Gutenboarding: #45610/#46670, or linking to the help/contact page in the editing toolkit.

Changes proposed in this Pull Request

  • Replaces client/languages with packages/languages.

Testing instructions

Unit tests check basic data is being loaded:

  • yarn workspace @automattic/languages test

Manually you can:

@lsl lsl requested review from p-jackson and a team October 23, 2020 06:04
@lsl lsl self-assigned this Oct 23, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 23, 2020
@lsl lsl mentioned this pull request Oct 23, 2020
46 tasks
@matticbot
Copy link
Contributor

matticbot commented Oct 23, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~6896 bytes added 📈 [gzipped])

name                   parsed_size           gzip_size
entry-gutenboarding       +15638 B  (+0.9%)    +1656 B  (+0.3%)
entry-main                +15619 B  (+1.2%)    +1845 B  (+0.5%)
entry-login               +15619 B  (+1.8%)    +1718 B  (+0.7%)
entry-domains-landing     +15619 B  (+2.8%)    +1677 B  (+1.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~50 bytes added 📈 [gzipped])

name            parsed_size           gzip_size
woocommerce           +34 B  (+0.0%)      +18 B  (+0.0%)
settings              +34 B  (+0.0%)       +6 B  (+0.0%)
account               +19 B  (+0.0%)       +4 B  (+0.0%)
themes                +15 B  (+0.0%)       +3 B  (+0.0%)
theme                 +15 B  (+0.0%)       +3 B  (+0.0%)
site-purchases        +15 B  (+0.0%)       +3 B  (+0.0%)
purchases             +15 B  (+0.0%)       +4 B  (+0.0%)
plugins               +15 B  (+0.0%)       +3 B  (+0.0%)
pages                 +15 B  (+0.0%)       +6 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~16 bytes added 📈 [gzipped])

name                                                     parsed_size           gzip_size
async-load-quick-language-switcher                             +19 B  (+0.0%)       +4 B  (+0.0%)
async-load-design-playground                                   +19 B  (+0.0%)       +3 B  (+0.0%)
async-load-design                                              +19 B  (+0.0%)       +2 B  (+0.0%)
async-load-calypso-layout-community-translator-launcher        +19 B  (+0.1%)       +3 B  (+0.0%)
async-load-calypso-components-community-translator             +19 B  (+0.2%)       +4 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@yuliyan yuliyan left a comment

Choose a reason for hiding this comment

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

I think it'd be better to stick with the current approach and have languages-meta.json data to be downloaded on each initial build (e.g. in the prepare script). And to avoid the data being constantly updated with other commits, languages-meta.json should also be git ignored.

Since the package depends mainly on that data file, from developer experience perspective, it's a good idea to have a data file that it can fallback to when it's unable to reach the download url (e.g. when offline). Here's the Webpack config in the current implementation that's used to replace the path to the JSON file.

Copy link
Member

@yuliyan yuliyan left a comment

Choose a reason for hiding this comment

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

After having a discussion with @lsl, we came to the conclusion that languages data doesn't change that frequently so it's not really needed. Downloading the languages data on each build will save the few extra steps to manually update the data, but it has some other downsides:

  • The downloaded data can't be validated properly
  • Relies on downloading the file, which could block the builds
  • It will require overcomplicated implementation with no real benefit

Having these in mind, we should stick with manual approach so we can have better control over the languages data updates and therefore the suggestion from my previous comment should be ignored.


I've noticed that there are some leftovers related to the old languages module:

Is there any reason to be keeping these?

Also left a few small comments, but other than that, I've tested the package and it LGTM.

packages/languages/bin/download.js Outdated Show resolved Hide resolved
packages/languages/src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Good comments above, but just dropping in to say this lgtm too

@lsl
Copy link
Contributor Author

lsl commented Oct 29, 2020

@yuliyan / @p-jackson thanks for the feedback/input, I've made a few changes:

  • Cleaned up the old files in client/languages. I was concerned these were still being referenced for type info. @p-jackson suggested exporting the types directly and changing client/languages/index.js to client/languages/index.ts so the types can be exported this seems to have worked.
  • Switched to ts in the package itself
  • Replaced transpile call with tsc calls directly (and corresponding configs)
  • Rebased after the language-picker was merged (Wanted to test that out locally)
  • Exported languages by default (and also as named - is this a problem? it made the bridge code a little simpler in client/langauges.)
  • Made the download script error by default, we could almost replace this with a wget call I think.

@yuliyan
Copy link
Member

yuliyan commented Oct 29, 2020

  • Made the download script error by default, we could almost replace this with a wget call I think.

Sounds good, but I'm not sure if wget has a good cross platform support (e.g. on Windows).

@lsl
Copy link
Contributor Author

lsl commented Oct 30, 2020

Felt leaving the old languages file around was a bit untidy so I've updated all the old references to point at the new package and removed it completely.

Tested:

  • community translator
  • my settings language selector
  • new language picker

Couldn't test

  • master bar quick language switcher (what is this?)
  • why the ci builds are failing

@p-jackson
Copy link
Member

17:34:29  FAIL components/translator-invite/test/utils.js
17:34:29    ● Test suite failed to run
17:34:29  
17:34:29      Cannot find module 'languages' from 'components/translator-invite/test/utils.js'
17:34:29  
17:34:29        14 | } );
17:34:29        15 |
17:34:29      > 16 | jest.mock( 'languages', () => ( {
17:34:29           |      ^
17:34:29        17 |   languages: [
17:34:29        18 |     {
17:34:29        19 |       value: 1,
17:34:29  
17:34:29        at Resolver.resolveModule (../node_modules/jest-resolve/build/index.js:307:11)
17:34:29        at Object.<anonymous> (components/translator-invite/test/utils.js:16:6)

This unit test failure is interesting, I wonder if it's something to do with no longer being allowed to implicitly import from the root of calypso repo. Maybe the path to mock just needs updated to calypso/languages or something. I assume it's the calypso/languages module the test is trying to mock ...

@p-jackson
Copy link
Member

And I'm not sure, but I think the ci/circleci: wait-calypso-live task fails if the build fails, which might fail if the unit tests fail.

@yuliyan
Copy link
Member

yuliyan commented Oct 30, 2020

  • master bar quick language switcher (what is this?)

image

It's enabled for support session, but you can also enable it with URL feature flag ?flags=quick-language-switcher.


  • why the ci builds are failing

As @p-jackson pointed above, it should be related to the failing unit tests. Let's try updating the mock here to point to the package @automattic/languages and return the array of languages directly, instead of mocking named export.


Also, package's own tests are also failing as they're still using named import here.

@lsl
Copy link
Contributor Author

lsl commented Nov 2, 2020

Maybe the path to mock just needs updated to calypso/languages or something. I assume it's the calypso/languages module the test is trying to mock ...

Close, I dropped this module completely now, @yuliyan mentioned the fix needed: updated to @automattic/languages and return a plain array by default. We could also just drop the mock completely but I think either way has a point.

@lsl
Copy link
Contributor Author

lsl commented Nov 2, 2020

rebase + conflict resolution

@lsl
Copy link
Contributor Author

lsl commented Nov 3, 2020

Squashed this - was becoming unruly to rebase against

  • Fixed bin/build-languages

@yuliyan I think I checked the build-languages script works properly but would appreciate a confirmation / double check

@yuliyan
Copy link
Member

yuliyan commented Nov 3, 2020

@lsl tested it and it works fine 🚀

I noticed that languages-meta.json has different formatting compared to the downloaded one (e.g. tabs vs spaces, territories array line breaks, etc.). It might be better to keep it in the raw formatting to avoid unreadable diff with the next update.

@yuliyan
Copy link
Member

yuliyan commented Nov 4, 2020

One more thing - I checked and it seems like revision property is not being referenced anywhere and since the data here won't be updated in sync with translations revisions constantly, the value will become obsolete pretty quick.

With that being said, I'd suggest removing it from the generated languages-meta.json file, as it will at least reduce the file size.

Ref. D52271-code

- Package tidy up
- Remove more of the old languages setup, replace with ts
- Always error / exit 1 on failure
- This script is no longer run as part of the build.
- Add languages as default export
- Replace transpile call with tsc directly
- Remove old languages dir completely and replace usage
- Use same lodash version as calypso
- Missed test import after default export change
- Test fix - update mock of languages import
- Use new package in bin/build-languages.js
@p-jackson
Copy link
Member

Rebased and force pushed

@lsl lsl merged commit b5da6eb into master Nov 8, 2020
@lsl lsl deleted the add/languages-package branch November 8, 2020 23:29
@matticbot matticbot removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs i18n Review labels Nov 8, 2020
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.

5 participants