-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Extract map for i18n strings to use for directed load of i18n strings per view. #6051
Conversation
Contained is the proof of concept webpack plugin. I realized after building that the plugin I did is not following the WebPack 4 plugin api (still works, just throws deprecated warnings). Should be fairly trivial to convert (I hope to convert to webpack 4 if needed, however it probably could be tweaked to support both). The pull isn't done, there still needs to be the php side of things done. But I thought I'd get this so it can be iterated on. |
9955aa1
to
68b1a5f
Compare
Alright, now I have added the php component for this and as far as I can tell it should work swimmingly. I still have to test with actual translations but I think I have enough for the POC now so we can review the current work and decide if this is a process we want to iterate on. Once that decision is made (along with any requested tweaks), I'll go ahead and clean up code style and add some phpunit tests for the php side of things, but I could use some help with the javascript tests because I haven't done many of those. For the php side, although it still in some ways is gutenberg specific, I still think it gives a bit of an idea of how things maybe could happen in WP core. Whether that gets generalized more before merging to gutenberg or gutenberg ships with a variation of this and the generalization takes place in the related trac ticket in tandem... that's up to you guys :) |
So in testing I'm seeing the locale data get built okay, but I think there may be an issue with where So only some things are getting translated (even though it looks like the strings ARE getting registered). Since things do work on master for the translation I'm testing with I'm comparing the two branches to see what's happening. |
K figured it out. Apparently it was due to a couple things:
After that I noticed that translations would get used only when a component was updated in the view. Traced that to where So, the first point I made above does sound like a possible bug with usage of |
Master doesn't pass an argument through for domain because those strings are assumed to be in the default domain (see default argument).
Either that, or immediately before the script using the strings (passing |
Hmm, however with |
So after the meeting today, I'm wondering if I should do the webpack plugin as a separate pull request on |
I think eventually we'd want it to live in the While core still uses Webpack 3, Gutenberg uses Webpack 4 and I think it would be fine to target only Webpack 4. |
As far as guarantees, sure. Conceptually it seems to invert awareness of scripts to be less of a handle associating their own strings, and more of the i18n script knowing which other scripts exist on the current screen. Not saying one or the other is better, but as traditionally with scripts defining their own dependencies, it seemed more intuitive to me that the relationship be with locale data set on the script using them. |
Right and this gets to the comment @youknowriad made in chat about ensuring |
I'm okay with that, sure. It gets a bit weird so far as what's expected to be passed to |
So for accumulative I see that the locale data is assigned to the options object per:
So would we just need to merge additional incoming localeData for the domain on subsequent calls to |
Right, I think this is a change we'd make at the package level right?
I can't think of a usecase offhand. |
Here's the pull on packages/i18n: WordPress/packages#105 |
I'm also not sure whether |
Yeah, we probably don't want to commit it, as it's a build artifact. |
f35bf76
to
3a8a8b2
Compare
Okay, apologies but I used
|
I use |
- removed functions I added in previous commit and moved that functionality into new class - introduce `gutenberg_register_script_i18n` function for registering a handle,chunk_name,domain. - hooked in on `print_scripts_array` to do some jit on the fly setting up of translation data for setting wpi18n via wp_add_inline_script(). This also will also setup domain specific (not required for gutenberg, but gives an example of what maybe could be done in core.
…ment - also hardcodes the handle to `wp-i18n` so that translations are swapped out immediately on initial load (timing is everything).
- this will use `tapable.hooks` if its present, otherwise `tapable.plugin` - fix linting errors - update translation-map.json (includes missing sprintf strings).
this is a build artifact so it should not be tracked in the repo.
3a8a8b2
to
30c4357
Compare
…s translations strings associated with it also some code style fixes
I missed that the format of the msgID for these strings includes the context string.
strings.push( args[ 1 ].value + '\u0004' + args[ 0 ].value ); | ||
break; | ||
case '_nx': | ||
strings.push( args[ 3 ].value + '\u0004' + args[ 0 ].value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had noticed that the msgID format for _x
and _nx
strings is {context}\u0004{singular_string}
so I corrected that. This took care of a few strings that weren't translating in my tests.
The latest round of changes:
|
So one evolution of this I'm thinking about is removing the need to require a
Then the Thoughts? |
- i18n-map-extractor.js now accepts an aliases property which can be used to map chunk name to script handle name. This results in a translation string map of handle-name > strings. - modified webpack.config.js to provide the aliases object. - simplify php side of things so that strings only need to be registered to handle.
I decided to go ahead and implement the changes mentioned in my previous comment. I think php side, this aligns better with the work being done in https://core.trac.wordpress.org/ticket/20491. |
This accounts for webpack configurations where there are multiple configurations setup and each one calls the plugin. It ensures that the map is built accumulatively.
I'm not sure that this is going to land in GB. We're using a variation of this for Event Espresso. I'm wondering if anyone would find benefit if I publish the webpack plugin as a package in our repo? |
Is there a reason for this feeling aside from needing support in pushing it forward? Is it still on a trajectory to be an official core module? |
It's hard to communicate sentiment via text so let me clarify that my earlier statement wasn't intended to communicate I'm bummed or upset about it. I'm just communicating my perception of the state of things from conversations that have happened so far in the i18n chats in slack for wp core. I think this has served as a good example of what can be done but I'm not sure it will land as is (especially the gb php pieces). I'm fine with leaving this pull open for reference for now (or it might get iterated on once there's a clearer direction for what needs to happen). Currently Event Espresso is using the webpack plugin as is right now in our own gutenberg integration work (with some improvements we've discovered it needed). I'm fine with dogfooding it for our needs for the time being while waiting for WP core to come up with a solution. However, as stated in my comment, I'm just wondering if there'd be value in us releasing it as an npm package via our (ee's) repo in case theirs others that could benefit from it - I'd also be fine with submitting it as a pull for the WordPress/packages but understand any reluctance to doing so being it's not known clearly what the direction will be for wpi18n yet. |
If I go by lack of feedback on it outside of the GB team I would say its not on trajectory to be an official core module. |
Is it safe to say that this could be considered a post-5.0 enhancement? Or is it blocking or backward-incompatible in any way? |
tldr; this doesn't block merge. The intent behind this was to complement what GB introduced with the @wordpress/i18n package in that it provides a mechanism for plugins to implement wp.i18n within their javascript. Since the merge of Gutenberg brings this to WP core, the goal was to come up with a suitable api for plugins to use as well once something landed in wp core (in the merge). There was supposed to be further discussion on this (and other proposed solutions) in a trac ticket (and wpi18n slack but those meetings petered out) and I still think that should happen at some point. I still don't know what the plans are for translating GB strings once GB lands in core? Are GB strings just going to be extracted to php a-la pot-to-php? Will it be expected that plugins with javascript i18n will do something similar? I don't think this necessarily holds up the merge for 5.0 but I do think that there's still a glaring hole in what plugins should be doing for i18n in javascript. In the js projects I'm involved in right now, I use a variation of what I proposed in here to manage our i18n strings, but it'd definitely be better if there was a canonical api that WP offered. I'm fine with this pull being bumped or even closed. If so, I'll probably just release the plugin I did on npm separately (will make it easier for us and others to pull in to projects if/as needed :) ) |
Yeah, there's a few localization things which I think would fall under the same realm of "needs to be done, but not blocking for a 5.0", including another I filed at #9846 and have since temporarily discontinued putting effort toward. For 5.0, I think the pot-to-php workflow is a reasonable stop-gap solution. At this point my main concern would be any foreseen incompatibilities that might occur in the eventual implementation (i.e. necessary for an API freeze). |
This uses several processes to allow translatable strings to be extracted directly from JS files and converted to PHP where they can be discovered and parsed by GlotPress. One downside of this current implementation is that the `gutenberg_get_jed_locale_data` function grabs all of the strings from the entire text domain. The `wordcamporg` domain currently has ~1,300 strings, all of which get output as an inline JS variable in the page document. Hopefully this will be solved in the future with something like this: WordPress/gutenberg#6051
Closing this in favor of the work being done here: https://core.trac.wordpress.org/ticket/45103 |
Description
Ongoing work. See #6015 for details.
Types of changes
When complete this will:
Checklist: