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

Make gettext functions filterable #12517

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@swissspidy
Copy link
Member

commented Dec 2, 2018

Description

Fixes #12516 by applying filters to all the gettext function in the i18n package.

How has this been tested?

Manually added some filters to these hooks.

Types of changes

New feature (non-breaking change which adds functionality): added new filters.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

Ugh. Travis CI says npm run check-local-changes failed, but locally it works just fine.

@aduth

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

Personally, I don't find this to be a pressing enhancement to consider, and while normally I'd not have such reservations, the translate functions are very hot paths. applyFilters is not especially "expensive", but without a use-case beyond broad flexibility and feature parity, it doesn't feel very compelling.

Alternatively, we could consider optimizing for the assumption that a filter will not be present in the majority of usage, and use a monkey-patching technique in overriding the translation functions when a filter is applied (see #8602 (comment)).

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2018

Yeah it's definitely not a pressing enhancement, but sure something that will come up when JS I18N is getting more traction now with 5.0 and beyond. Parity and flexibility for devs is definitely important to some degree, IMO.

Your monkey-patching idea sounds interesting, but I guess we would need to check first whether it would be worth the effort. I was thinking about doing some benchmarks or something, but I wouldn't know where to start there.

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

For the record, we discussed some use cases at WCUS, mainly #12407 and https://core.trac.wordpress.org/ticket/45425#comment:10. It's certainly not ideal that the user experience for certain locales got worse.

As for the technique used here, we could do some benchmarks to see whether monkey-patching should be explored or not. Although that could also be done in a later version.

* @param {string} text Text to translate.
* @param {string} domain Text domain. Unique identifier for retrieving translated strings.
*/
return applyFilters( 'i18n.gettext', translation, text, domain );

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 14, 2018

Member

@aduth - we probably should also optimize applyFilters to return early when there are no hooks to process. See:

https://github.com/WordPress/gutenberg/blob/master/packages/hooks/src/createRunHook.js#L22-L33

It makes me think that having to run filters on every re-render might be bad for the performance of the application.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 14, 2018

Member

Okey, I see #12517 (comment) which aligns with my comment.

This comment has been minimized.

Copy link
@aduth

aduth Dec 14, 2018

Member

Yeah, it's not too horrible though. Outside of some conditions and a one-time setup, it amounts to incrementing the runs each time it's called.

@gziolo

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

It looks like this is something that we need to decide about whether we continue working on this PR or we close it and keep it as a future reference when we decide it becomes necessary.

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

@gziolo @aduth What's the way forward here? A discussion at the next weekly meeting together with some core i18n / polyglots people involved?

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

A discussion at the next weekly meeting together with some core i18n / polyglots people involved?

Which weekly meeting are you referring to? I can have it included it in the agenda of tomorrow's JavaScript chat if you'd like.

@swissspidy

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Yeah that could work.

@aduth

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.