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

i18n: Add create-i18n function #21182

Merged
merged 18 commits into from
Mar 27, 2020
Merged

i18n: Add create-i18n function #21182

merged 18 commits into from
Mar 27, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Mar 26, 2020 •
edited
Loading

Closes #20318 which stagnated and was apparently abandoned. Part of #19210.

Quoting:

Description

As per the proposed solution in #19210 I've refactored the @wordpress/i18n module to expose an instantiable class. e.g.

import { I18N } from '@wordpress/i18n';

const strayaLocale = {
	hello: [ 'gday' ],
};

const frenchLocale = {
	hello: [ 'bonjour' ],
};

const straya = new I18N();
const french = new I18N();

straya.setLocaleData( strayaLocale );
french.setLocaleData( frenchLocale );

expect( straya.__( 'hello' ) ).toEqual( 'gday' );
expect( french.__( 'hello' ) ).toEqual( 'bonjour' );

There are no changes to the existing external interface of the module other than the addition of the I18N class.

How has this been tested?

Via the existing suite of unit tests and an additional unit test.

Screenshots

N/A

Types of changes

New feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@sirreal sirreal added [Package] i18n /packages/i18n [Type] Feature New feature to highlight in changelogs. labels Mar 26, 2020
@sirreal sirreal requested a review from swissspidy as a code owner March 26, 2020 19:13
@sirreal sirreal self-assigned this Mar 26, 2020
@sirreal sirreal mentioned this pull request Mar 26, 2020
Closed
6 tasks
aduth
aduth approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Thanks 👍

packages/i18n/src/create-i18n.js Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Mar 26, 2020 •
edited
Loading

Size Change: +82 B (0%)

Total Size: 857 kB

Filename Size Change
build/i18n/index.js 3.57 kB +82 B (2%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.45 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.23 kB 0 B
build/block-library/index.js 110 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.5 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.5 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.25 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91 kB 0 B
build/edit-post/style-rtl.css 8.43 kB 0 B
build/edit-post/style.css 8.43 kB 0 B
build/edit-site/index.js 6.73 kB 0 B
build/edit-site/style-rtl.css 2.91 kB 0 B
build/edit-site/style.css 2.9 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.57 kB 0 B
build/edit-widgets/style.css 2.57 kB 0 B
build/editor/editor-styles-rtl.css 423 B 0 B
build/editor/editor-styles.css 426 B 0 B
build/editor/index.js 42.8 kB 0 B
build/editor/style-rtl.css 3.38 kB 0 B
build/editor/style.css 3.38 kB 0 B
build/element/index.js 4.44 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 6.95 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 622 B 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.7 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 781 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.5 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

sirreal
sirreal commented Mar 26, 2020
View reviewed changes
Copy link
Member Author

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

data not optional in setLocaleData.

packages/i18n/src/default-i18n.js Show resolved Hide resolved
packages/i18n/src/create-i18n.js Show resolved Hide resolved
@sirreal
Copy link
Member Author

sirreal commented Mar 26, 2020

This bit of feedback slipped through the cracks. I can address it here.

I disagree with exporting the defaultRegistry.

#20318 (comment)

IMO: Let's not export the default instance. If we need to later, we can add it.

#20318 (comment)

@sirreal
Copy link
Member Author

sirreal commented Mar 26, 2020

This bit of feedback slipped through the cracks. I can address it here.

I disagree with exporting the defaultRegistry.

#20318 (comment)

IMO: Let's not export the default instance. If we need to later, we can add it.

#20318 (comment)

On second thought, this is in a good place to merge and these changes can be made in follow-up PRs.

@aduth
Copy link
Member

aduth commented Mar 26, 2020

I don't see where we're exporting the default instance? It would be good to avoid that if we are.

@sirreal
Copy link
Member Author

sirreal commented Mar 26, 2020

I don't see where we're exporting the default instance? It would be good to avoid that if we are.

It's here:

https://github.com/WordPress/gutenberg/blob/c0e49d9fe4a44a63f64acc0ab9bf819d2fa9a9ff/packages/i18n/src/index.js#L3

Would you prefer we remove that here?

@aduth
Copy link
Member

aduth commented Mar 26, 2020 •
edited
Loading

I think the original issue was different, where previously with:

https://github.com/jameslnewell/gutenberg/blob/3059db63ccd7d63f895ce3abf76bc709e38a4f56/packages/i18n/src/index.js#L7

...combined with:

https://github.com/jameslnewell/gutenberg/blob/3059db63ccd7d63f895ce3abf76bc709e38a4f56/packages/i18n/src/default-i18n.js#L6

...it would result in wp.i18n.i18n (import { i18n as defaultI18n } from '@wordpress/i18n';), which is what we didn't want. We're no longer exporting that default instance in its entirety from default-i18n.js (fda6948), so I don't believe it's a problem.

@sirreal
Copy link
Member Author

sirreal commented Mar 27, 2020

I had understood that the methods on the shared i18n singleton would not be provided by the package at all, and instead WordPress would instantiate and provide a default here:

https://github.com/WordPress/WordPress/blob/f4f1184debce6829fde01010c0df90c33f9163f8/wp-includes/script-loader.php#L276

That would be a breaking change in this package's API and would address partially #8981.

I'll leave the default translation functions.

@sirreal sirreal force-pushed the add/i18n-create-instance branch from 71fa5a8 to 87bb2f5 Compare March 27, 2020 22:28
@sirreal sirreal merged commit 0e18615 into master Mar 27, 2020
@sirreal sirreal deleted the add/i18n-create-instance branch March 27, 2020 23:04
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@swissspidy swissspidy Awaiting requested review from swissspidy

@youknowriad youknowriad Awaiting requested review from youknowriad

@jsnajdr jsnajdr Awaiting requested review from jsnajdr

Assignees

@sirreal sirreal

Labels
[Package] i18n /packages/i18n [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Milestone
Gutenberg 7.9
Development

Successfully merging this pull request may close these issues.
2 participants
@sirreal @aduth