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

Onboarding: some copy isn't appearing in the list of strings to be translated #47884

Closed
p-jackson opened this issue Nov 30, 2020 · 12 comments · Fixed by #48074
Closed

Onboarding: some copy isn't appearing in the list of strings to be translated #47884

p-jackson opened this issue Nov 30, 2020 · 12 comments · Fixed by #48074
Labels
[Goal] New Onboarding previously called Gutenboarding i18n Launch

Comments

@p-jackson
Copy link
Member

Steps to reproduce

What I expected

All copy translated :)

What happened instead

Copy like "Site launch steps" isn't translated
Also on translate.wordpress.org you see that even the English strings are missing

Screenshot / Video

Ignore the red box, this bug is just about what the copy the arrows are pointing to.
image

Context / Source

pbAok1-1II-p2#comment-3455

@p-jackson p-jackson added i18n Launch [Goal] New Onboarding previously called Gutenboarding labels Nov 30, 2020
@p-jackson p-jackson mentioned this issue Dec 1, 2020
46 tasks
@emilyaudela
Copy link
Contributor

emilyaudela commented Dec 1, 2020

Just wanted to note that we have the translations for all of these strings and they were appearing translated earlier last week, when the first reviewers reviewed the content (on November 23-25). It looks like they only started appearing in English the last few days starting on the 27th...

@lsl
Copy link
Contributor

lsl commented Dec 2, 2020

@yuliyan would you be able to take a look into this? Even if its just to educate me on where translations for ETK are coming from? These strings are translated on translate.wordpress.com but not present for translate.wordpress.org. Is this down to creating plugin releases perhaps?

@yuliyan
Copy link
Member

yuliyan commented Dec 2, 2020

@lsl, ETK translations are coming from translate.wordpress.org. Our internal i18n-deploy tool downloads the translations, builds language packs and commits them to WPcom i18n svn.

The problem with the missing translation seems to be that the originals for the mentioned strings have disappeared from https://translate.wordpress.org/projects/wp-plugins/full-site-editing/stable/. I found that this have happened with https://plugins.trac.wordpress.org/changeset?old_path=%2Ffull-site-editing&old=2424109&new_path=%2Ffull-site-editing&new=2424109&sfp_email=&sfph_mail=#file8.

I tried downloading that version of the plugin and extracting translatable strings manually with wp i18n make-pot (AFAIK, that's how strings for themes and plugins on .org are extracted and imported into GP) and those strings were indeed missing from the generated POT file, which is why those were removed from GlotPress.

The reason why those strings are not being extracted is that wp i18n make-pot is using Peast to parse JavaScript files, which currently doesn't support TypeScript. This means that .ts(x) files would be ignored, and therefore strings from those files will not be extracted. However, since it scans the entire plugin directory, it doesn't only extract from source files, but also from the compiled bundle/dist files, which is where the missing strings were previously extracted from.

Looking closely at the editor-site-launch.js dist file, I noticed that prior to 2.8.8, in the minified code the callee names of the translation expressions were preserved (e.g. Object(y.__)("Select a domain","full-site-editing")), and after the use of useI18n hook from react-i18n was introduced in 2.8.8, that has changed (e.g. o("Select a domain","full-site-editing")). And those names (e.g. __, _n, _x, _nx) are exactly what's being used to determine which strings should be extracted.

For a quick fix workaround, I'd suggested to temporary replace destructing the result of the useI18n hook with const i18n = useI18n(); and call translate functions as member expressions (i.e. i18n.__( 'translate me' ) ). However, I'd recommend to only use this until we come up with something better, as it doesn't seem like a very reliable approach since it depends mostly on current behaviour of the bundler & the minifier.

@yuliyan
Copy link
Member

yuliyan commented Dec 2, 2020

I thought of another option - maybe we can exclude translation function names from terser's mangle options, e.g.:

mangle: {
	properties: {
		regex: /^((?!(__|_n|_nx|_x)).)*$/,
	}
}

Though, I'm not very experienced with terser and I'm not sure if it's ok to use it like that. @scinos, do you see any problem in that approach?

@simison
Copy link
Member

simison commented Dec 3, 2020

Somehow I remember doing tests long ago where the extractor could read mangled code just fine. How do __() calls in Jetpack bundles look like, for example?

@p-jackson
Copy link
Member Author

@simison I believe the extractor has been written to deal with a few different ways the code can be mangled: Object(y.__)("hello, world") and something["__"]("hello, world") both work. It's only the fact that __ can now be a local variable with react-i18n that it breaks. What's funny is that this is one of the advantages of React hooks, they can make the bundled code even smaller, it just works too good in this case! 😆

IMO I think excluding translation functions from minification is the best approach, I think too much of our toolchain assumes it will be able to find __ symbols in the code.

@lsl
Copy link
Contributor

lsl commented Dec 3, 2020

IMO I think excluding translation functions from minification is the best approach

I think this has been done already for something in the past, maybe i18n-calypso's translation function? I wonder if we need to do the same.

@simison
Copy link
Member

simison commented Dec 4, 2020

Gotcha, thanks for explaining @p-jackson. Let's treat this as a priority so that it doesn't block AB testing.

@yuliyan
Copy link
Member

yuliyan commented Dec 4, 2020

IMO I think excluding translation functions from minification is the best approach, I think too much of our toolchain assumes it will be able to find __ symbols in the code.

Ideally, we'd be extracting the strings from the source files, and not the compiled files, which would make it less dependant on the build tools we use and give better source references in GP. However, it looks like Peast doesn't plan on adding TypeScript support for now, so unless wp i18n command shifts away from Peast, I don't see that being possible.

@simison
Copy link
Member

simison commented Dec 4, 2020

FYI @Automattic/team-calypso this thread might interest you.

@jsnajdr
Copy link
Member

jsnajdr commented Dec 4, 2020

I agree that preventing mangling of __ and friends is the most viable solution. The right config option for Terser seems to be:

mangle: {
  reserved: [ '__', '_n', '_nx', '_x' ]
}

We want to prevent mangling local variables initialized with useI18n, right? mangle.properties doesn't seem to be relevant here. I'm not even sure how mangle.properties is supposed to work without breaking the app -- maybe it's mangling only properties of objects that are strictly local values and where all usages can be traced?

@mirka
Copy link
Member

mirka commented Jan 6, 2021

I wanted to note that this process will fail to pick up translators comments.

I also have TS code that needs to be translated, and I've been trying out running make-pot on a prod build generated by calypso-build, as I believe some of you are doing. It works great, expect translators comments don't show up because comments are stripped from the build.

It only works when Terser is configured to retain the translators comments, and also beautify: true, because it needs line breaks for it to be parsed correctly. Not sure if this is something that needs to be addressed in calypso-build, or if people needing make-pot parsing should just custom generate their own parseable builds. I'm going with the latter for now, but just a heads up to you all as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] New Onboarding previously called Gutenboarding i18n Launch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants