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

Move localizeUrl to new i18n-utils package #46998

Merged
merged 6 commits into from
Nov 11, 2020
Merged

Move localizeUrl to new i18n-utils package #46998

merged 6 commits into from
Nov 11, 2020

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Nov 2, 2020

Changes proposed in this Pull Request

  • Moves lib/i18n-utils' localizeUrl function to a new package in packages/i18n-utils

Testing instructions

localizeUrl tests were included in the move:

yarn test-client client/lib/i18n-utils
yarn workspace @automattic/i18n-utils test

Manual testing involves checking localizeUrl usage still works as expected for example:


@matticbot
Copy link
Contributor

@lsl lsl self-assigned this Nov 2, 2020
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D52130-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

matticbot commented Nov 2, 2020

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

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

name                   parsed_size           gzip_size
entry-main                  +291 B  (+0.0%)     +103 B  (+0.0%)
entry-login                 +291 B  (+0.0%)      +90 B  (+0.0%)
entry-gutenboarding         +291 B  (+0.0%)      +70 B  (+0.0%)
entry-domains-landing       +291 B  (+0.1%)      +82 B  (+0.1%)

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

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.

@ramonjd ramonjd requested a review from a team November 5, 2020 01:06
@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 Nov 5, 2020
@ramonjd ramonjd requested a review from a team November 5, 2020 01:07
@ramonjd ramonjd added the i18n label Nov 5, 2020
@ramonjd ramonjd mentioned this pull request Nov 5, 2020
46 tasks
client/lib/wporg/index.js Outdated Show resolved Hide resolved
packages/i18n-utils/jest.config.js Outdated Show resolved Hide resolved
packages/languages/bin/download.js Outdated Show resolved Hide resolved
packages/languages/bin/download.js Outdated Show resolved Hide resolved
packages/languages/jest.config.js Outdated Show resolved Hide resolved
@lsl
Copy link
Contributor Author

lsl commented Nov 5, 2020

@yuliyan / @scinos

Parts of this i18n-utils package make more sense to be part of packages/languages: e.g. getLanguage( langSlug ) just performs a lookup on the language data with a fallback to parent locale if available, and isTranslatedIncompletely just returns a value of the language looked up by getLanguage

The remaining parts make more sense to be part of package/i18n-calypso. e.g. canBeTranslated, translationExists, isMagnificentLocale are all very a8c specific or depend on i18n-calypso to get the current locale (like localizeUrl)

My thinking is maybe to split i18n-utils up between the two existing packages and drop the notion of a new one. Does this sound ok or is there still a push to drop i18n-calypso in favor of @wordpress/i18n? In which case it may be wise to keep i18n-utils in play with some/none of it moved into languages. It would still depend on i18n-calypso but that can be dropped as soon as there is a replacement solution for getLocaleSlug (the only problematic dep.)

@lsl
Copy link
Contributor Author

lsl commented Nov 5, 2020

@scinos I copy and pasted most of these config files / settings from elsewhere and have only looked into a handful of what each thing does so I very much appreciate the audit! 🍻

Most of your comments were on code from the other PR #46709 so I've fixed there and rebased here. Thanks!

@yuliyan
Copy link
Member

yuliyan commented Nov 5, 2020

Parts of this i18n-utils package make more sense to be part of packages/languages: e.g. getLanguage( langSlug ) just performs a lookup on the language data with a fallback to parent locale if available, and isTranslatedIncompletely just returns a value of the language looked up by getLanguage

That makes sense and I think moving them into @automattic/languages is fine. As for the remaining helpers, I like the idea of having them separate from i18n-calypso better. With @wordpress/i18n we would be forced to have them in a separate package anyway, so I think it will make the transition more smooth.

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.

Looks good so far. I realise it's still a WIP, just left a couple notes. Let me know if there's anything I can help with.

packages/i18n-utils/README.md Outdated Show resolved Hide resolved
packages/languages/src/index.ts 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.

I haven't quite finished my review yet: I can't seem to build calypso locally, and it also looks like there's a change where the new package isn't doing some sort of lazy load of something out of the config and I want to get to the bottom of that.

packages/i18n-utils/test/localize-url.js Show resolved Hide resolved
packages/i18n-utils/test/localize-url.js Show resolved Hide resolved
packages/i18n-utils/test/localize-url.js Show resolved Hide resolved
packages/i18n-utils/src/locales.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@wp-desktop wp-desktop left a comment

Choose a reason for hiding this comment

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

WordPress Desktop CI Failure for job "wp-desktop-mac".

@lsl please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

@wp-desktop wp-desktop dismissed their stale review November 11, 2020 01:12

wp-desktop ci passing, closing review

@lsl lsl marked this pull request as ready for review November 11, 2020 03:37
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.

Looks good 👍

@lsl
Copy link
Contributor Author

lsl commented Nov 11, 2020

This sort of non-side-by-side refactor is quite hard to do safely, caught a sloppy copy paste bug testing random links in the UI: 59dd2bc (#46998), and it just so happened that we were missing test coverage for the scenario that was introduced!

@lsl lsl requested a review from yuliyan November 11, 2020 05:52
@lsl lsl changed the title Add/i18n utils package Move localizeUrl to new i18n-utils package Nov 11, 2020
@lsl lsl merged commit 1446651 into master Nov 11, 2020
@lsl lsl deleted the add/i18n-utils-package branch November 11, 2020 06:10
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 11, 2020
@yuliyan
Copy link
Member

yuliyan commented Nov 11, 2020

Are #localizeUrl unit tests in lib/i18n-utils now redundant? If so, let's remove them in a follow-up PR before they start failing.

Edit: Please ignore my comment above, I was looking at an outdated version of the file.

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

Successfully merging this pull request may close these issues.

Gutenboarding: i18n
8 participants