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

Make i18n functions filter their return values #27966

Merged
merged 17 commits into from
Jan 28, 2021
Merged

Make i18n functions filter their return values #27966

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f9b484a
Make i18n functions filter their return values
leewillis77 Dec 31, 2020
99d3466
@wordpress/hooks local dependency
leewillis77 Jan 4, 2021
5176c06
Support for domain-specific filters as per PHP
leewillis77 Jan 4, 2021
1efa483
Move responsibility for creating applyFilters to default-i18n
leewillis77 Jan 5, 2021
4a3c2d7
Add tests
leewillis77 Jan 5, 2021
2866eb9
Merge branch 'master' into add/i18n-filters-2
leewillis77 Jan 6, 2021
883bc53
Merge branch 'master' into add/i18n-filters-2
leewillis77 Jan 7, 2021
a0a0a73
Flag dependency in tsconfig.json
leewillis77 Jan 12, 2021
e955c4c
package-lock changes
leewillis77 Jan 12, 2021
40cb141
Coding standards fix
leewillis77 Jan 14, 2021
a320295
Introduce i18n-filters test
leewillis77 Jan 21, 2021
815a422
Include i18n-filters test in CI task
leewillis77 Jan 21, 2021
f8eca3a
More consistent (not perfect) hook attachment
leewillis77 Jan 22, 2021
d6ae239
Test fix
leewillis77 Jan 23, 2021
8ab3436
Nit: type names start with a capital letter
leewillis77 Jan 26, 2021
f34fcf0
Add default i18n tests
leewillis77 Jan 26, 2021
f9650fe
Use type annotations rather than explicit conversion
leewillis77 Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,11 @@ function gutenberg_override_script( $scripts, $handle, $src, $deps = array(), $v
* `WP_Dependencies::set_translations` will fall over on itself if setting
* translations on the `wp-i18n` handle, since it internally adds `wp-i18n`
* as a dependency of itself, exhausting memory. The same applies for the
* polyfill script, which is a dependency _of_ `wp-i18n`.
* polyfill and hooks scripts, which are dependencies _of_ `wp-i18n`.
*
* See: https://core.trac.wordpress.org/ticket/46089
*/
if ( 'wp-i18n' !== $handle && 'wp-polyfill' !== $handle ) {
if ( ! in_array( $handle, [ 'wp-i18n', 'wp-polyfill', 'wp-hooks'], true ) ) {
sirreal marked this conversation as resolved.
Show resolved Hide resolved
$scripts->set_translations( $handle, 'default' );
}
}
Expand Down
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/i18n/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ _Parameters_

- _initialData_ `[LocaleData]`: Locale data configuration.
- _initialDomain_ `[string]`: Domain for which configuration applies.
- _hooks_ `[ApplyFiltersInterface]`: Hooks implementation.

_Returns_

Expand Down
1 change: 1 addition & 0 deletions packages/i18n/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
},
"dependencies": {
"@babel/runtime": "^7.12.5",
"@wordpress/hooks": "file:../hooks",
"gettext-parser": "^1.3.1",
"lodash": "^4.17.19",
"memize": "^1.1.0",
Expand Down
155 changes: 150 additions & 5 deletions packages/i18n/src/create-i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ const DEFAULT_LOCALE_DATA = {
*
* @see http://messageformat.github.io/Jed/
*/
/**
* @typedef {(domain?: string) => string} getFilterDomain
Copy link
Member

Choose a reason for hiding this comment

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

Nit: type names start with a capital letter: GetFilterDomain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks

* Retrieve the domain to use when calling domain-specific filters.
*/
/**
* @typedef {(text: string, domain?: string) => string} __
*
Expand Down Expand Up @@ -70,6 +74,9 @@ const DEFAULT_LOCALE_DATA = {
* language written RTL. The opposite of RTL, LTR (Left To Right) is used in other languages,
* including English (`en`, `en-US`, `en-GB`, etc.), Spanish (`es`), and French (`fr`).
*/
/**
* @typedef {{ applyFilters: (hookName:string, ...args: unknown[]) => unknown}} ApplyFiltersInterface
*/
/* eslint-enable jsdoc/valid-types */

/**
Expand All @@ -92,9 +99,10 @@ const DEFAULT_LOCALE_DATA = {
*
* @param {LocaleData} [initialData] Locale data configuration.
* @param {string} [initialDomain] Domain for which configuration applies.
* @param {ApplyFiltersInterface} [hooks] Hooks implementation.
* @return {I18n} I18n instance
*/
export const createI18n = ( initialData, initialDomain ) => {
export const createI18n = ( initialData, initialDomain, hooks ) => {
/**
* The underlying instance of Tannin to which exported functions interface.
*
Expand Down Expand Up @@ -147,24 +155,161 @@ export const createI18n = ( initialData, initialDomain ) => {
return tannin.dcnpgettext( domain, context, single, plural, number );
};

/** @type {getFilterDomain} */
const getFilterDomain = ( domain ) => {
if ( typeof domain === 'undefined' ) {
return 'default';
}
return domain;
};

/** @type {__} */
const __ = ( text, domain ) => {
return dcnpgettext( domain, undefined, text );
let translation = dcnpgettext( domain, undefined, text );
/**
* Filters text with its translation.
*
* @param {string} translation Translated text.
* @param {string} text Text to translate.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
if ( typeof hooks === 'undefined' ) {
return translation;
}
translation = String(
Copy link
Contributor

Choose a reason for hiding this comment

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

Converting the translation to always be of a string type seems a bit restrictive. Is there a reason not to allow it to be filtered into any type of value?

Copy link
Member

Choose a reason for hiding this comment

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

The type definitions / JSDoc say that it returns a string, so it makes sense to uphold that promise, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth removing the type conversion and updating the JSDoc? I think it will make the filters a lot more flexible, especially in a React context, where it might not be uncommon to need to return a React component directly as a result of the gettext call.

Copy link
Member

Choose a reason for hiding this comment

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

When used in a React component:

<span>{ __( 'Hello' ) }</span>

The return value of __() can be a generic ReactNode, which includes many possible value types, string being one of them.

It would be nice to keep this flexibility -- there are creative use cases that benefit from it.

The return type of hooks.applyFilters is unknown. If we want to coerce that into a string, we can use a type annotation:

translation = /** @type {string} */ hooks.applyFilters();

That affects only the type checker, and doesn't do runtime conversions like String() does.

Copy link
Contributor Author

@leewillis77 leewillis77 Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm more than happy to make this change (The String() calls always made me feel a little uneasy!) I've tried the annotation approach required here, but can't seem to make it compile, and unfortunately I don't know enough about the syntax to coax it into submission.

packages/i18n/src/create-i18n.js:179:3 - error TS2322: Type 'unknown' is not assignable to type 'string'.

179  	translation = /** @type {string} */ hooks.applyFilters(
     	~~~~~~~~~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnajdr It looks like applyFilters returns unknown rather than any which means (I think) that we can't just type hint it?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. The unknown type should be assignable to anything else, without constraints. I though that value as string is exactly the same thing as /** @type {string} */ value, but TypeScript playground reports error only for the second (jsdoc) one:

Screenshot 2021-01-28 at 10 58 45

@sirreal Do you have any insight about what's going on? Maybe there is some TS option that affects this?

Copy link
Member

Choose a reason for hiding this comment

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

@leewillis77 I found that this workaround works:

translation = /** @type {string} */ ( /** @type {*} */ applyFilters( ... ) );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnajdr Thanks. PR updated.

Copy link
Member

@sirreal sirreal Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

EDIT This is a response to a few previous comments the thread and focuses on the types. It does not take into account the other context and is not a request for changes.

Interesting. The unknown type should be assignable to anything else, without constraints.

@sirreal Do you have any insight about what's going on? Maybe there is some TS option that affects this?

Anything can be assigned to the unknown type, but unknown does not satisfy any other types. any is the type that satisfies all other type (and anything can be assigned to any.

You can read more about unknown in its announcement.

TypeScript 3.0 introduces a new top type unknown. unknown is the type-safe counterpart of any. Anything is assignable to unknown, but unknown isn’t assignable to anything but itself and any without a type assertion or a control flow based narrowing. Likewise, no operations are permitted on an unknown without first asserting or narrowing to a more specific type.

When we really don't have type information, especially at the boundaries of our typed application like a REST response or receiving data across untyped APIs like here, unknown makes that very clear and forces you to narrow with guards or type assertions. Before unknown, there was only any which was completely unsafe because you can put anything in and do anything with it.

The error is expected and the fix is what TypeScript recommends (this is a type assertion written in JSDoc). First, we assert the unknown is any, then we can narrow it to the type of our choosing because any:

translation = /** @type {string} */ ( /** @type {*} */ applyFilters( ... ) );

A (safer) alternative would be to use a type guard, but has a runtime cost:

const filteredTranslation = applyFilters( /* … */ );
return typeof filteredTranslation === 'string' ? filteredTranslation : translation; // we know it's a string

hooks.applyFilters( 'i18n.gettext', translation, text, domain )
);
return String(
hooks.applyFilters(
'i18n.gettext_' + getFilterDomain( domain ),
translation,
text,
domain
)
);
};

/** @type {_x} */
const _x = ( text, context, domain ) => {
return dcnpgettext( domain, context, text );
let translation = dcnpgettext( domain, context, text );
/**
* Filters text with its translation based on context information.
*
* @param {string} translation Translated text.
* @param {string} text Text to translate.
* @param {string} context Context information for the translators.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
if ( typeof hooks === 'undefined' ) {
return translation;
}
translation = String(
hooks.applyFilters(
'i18n.gettext_with_context',
translation,
text,
context,
domain
)
);
return String(
hooks.applyFilters(
'i18n.gettext_with_context_' + getFilterDomain( domain ),
translation,
text,
context,
domain
)
);
};

/** @type {_n} */
const _n = ( single, plural, number, domain ) => {
return dcnpgettext( domain, undefined, single, plural, number );
let translation = dcnpgettext(
domain,
undefined,
single,
plural,
number
);
if ( typeof hooks === 'undefined' ) {
return translation;
}
/**
* Filters the singular or plural form of a string.
*
* @param {string} translation Translated text.
* @param {string} single The text to be used if the number is singular.
* @param {string} plural The text to be used if the number is plural.
* @param {string} number The number to compare against to use either the singular or plural form.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
translation = String(
hooks.applyFilters(
'i18n.ngettext',
translation,
single,
plural,
number,
domain
)
);
return String(
hooks.applyFilters(
'i18n.ngettext_' + getFilterDomain( domain ),
translation,
single,
plural,
number,
domain
)
);
};

/** @type {_nx} */
const _nx = ( single, plural, number, context, domain ) => {
return dcnpgettext( domain, context, single, plural, number );
let translation = dcnpgettext(
domain,
context,
single,
plural,
number
);
if ( typeof hooks === 'undefined' ) {
return translation;
}
/**
* Filters the singular or plural form of a string with gettext context.
*
* @param {string} translation Translated text.
* @param {string} single The text to be used if the number is singular.
* @param {string} plural The text to be used if the number is plural.
* @param {string} number The number to compare against to use either the singular or plural form.
* @param {string} context Context information for the translators.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
translation = String(
hooks.applyFilters(
'i18n.ngettext_with_context',
translation,
single,
plural,
number,
context,
domain
)
);
return String(
hooks.applyFilters(
'i18n.ngettext_with_context_' + getFilterDomain( domain ),
translation,
single,
plural,
number,
context,
domain
)
);
};

/** @type {IsRtl} */
Expand Down
7 changes: 6 additions & 1 deletion packages/i18n/src/default-i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
/**
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import { createI18n } from './create-i18n';

const i18n = createI18n();
const i18n = createI18n( undefined, undefined, { applyFilters } );

/*
* Comments in this file are duplicated from ./i18n due to
Expand Down
59 changes: 59 additions & 0 deletions packages/i18n/src/test/create-i18n.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,63 @@ describe( 'createI18n', () => {
} );
} );

describe( 'i18n filters', () => {
Copy link
Member

Choose a reason for hiding this comment

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

There's one more thing that I thing is worth testing: interaction of the default i18n with the default hooks:

import { __ } from '@wordpress/i18n';
import { addFilter } from '@wordpress/hooks';

test( () => {
  addFilter( 'i18n.gettext', ... );
  expect( __( 'hello' ) ).toBe( ... );
} );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added packages/i18n/src/test/default-i18n.js. Thanks!

test( '__() calls filters', () => {
const i18n = createI18n( undefined, undefined, {
applyFilters: ( filter, translation ) => translation + filter,
} );
expect( i18n.__( 'hello' ) ).toEqual(
'helloi18n.gettexti18n.gettext_default'
);
expect( i18n.__( 'hello', 'domain' ) ).toEqual(
'helloi18n.gettexti18n.gettext_domain'
);
} );
test( '_x() calls filters', () => {
const i18n = createI18n( undefined, undefined, {
applyFilters: ( filter, translation ) => translation + filter,
} );
expect( i18n._x( 'hello', 'context' ) ).toEqual(
'helloi18n.gettext_with_contexti18n.gettext_with_context_default'
);
expect( i18n._x( 'hello', 'context', 'domain' ) ).toEqual(
'helloi18n.gettext_with_contexti18n.gettext_with_context_domain'
);
} );
test( '_n() calls filters', () => {
const i18n = createI18n( undefined, undefined, {
applyFilters: ( filter, translation ) => translation + filter,
} );
expect( i18n._n( 'hello', 'hellos', 1 ) ).toEqual(
'helloi18n.ngettexti18n.ngettext_default'
);
expect( i18n._n( 'hello', 'hellos', 1, 'domain' ) ).toEqual(
'helloi18n.ngettexti18n.ngettext_domain'
);
expect( i18n._n( 'hello', 'hellos', 2 ) ).toEqual(
'hellosi18n.ngettexti18n.ngettext_default'
);
expect( i18n._n( 'hello', 'hellos', 2, 'domain' ) ).toEqual(
'hellosi18n.ngettexti18n.ngettext_domain'
);
} );
test( '_nx() calls filters', () => {
const i18n = createI18n( undefined, undefined, {
applyFilters: ( filter, translation ) => translation + filter,
} );
expect( i18n._nx( 'hello', 'hellos', 1, 'context' ) ).toEqual(
'helloi18n.ngettext_with_contexti18n.ngettext_with_context_default'
);
expect( i18n._nx( 'hello', 'hellos', 1, 'context', 'domain' ) ).toEqual(
'helloi18n.ngettext_with_contexti18n.ngettext_with_context_domain'
);
expect( i18n._nx( 'hello', 'hellos', 2, 'context' ) ).toEqual(
'hellosi18n.ngettext_with_contexti18n.ngettext_with_context_default'
);
expect( i18n._nx( 'hello', 'hellos', 2, 'context', 'domain' ) ).toEqual(
'hellosi18n.ngettext_with_contexti18n.ngettext_with_context_domain'
);
} );
} );

/* eslint-enable @wordpress/i18n-text-domain, @wordpress/i18n-translator-comments */
5 changes: 4 additions & 1 deletion packages/i18n/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,8 @@
"rootDir": "src",
"declarationDir": "build-types"
},
"include": [ "src/**/*" ]
"references": [
{ "path": "../hooks" },
],
"include": [ "src/**/*" ],
}