chore(web-forms#825): allow for forms app translation in transifex - #1692
chore(web-forms#825): allow for forms app translation in transifex#1692garethbowen wants to merge 13 commits into
Conversation
🦋 Changeset detectedLatest commit: ff8f41c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| `${localesDir}/${locale}.json`, | ||
| JSON.stringify(translations, null, 2) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I added this because forms doesn't support all the languages that central does. Without this the script fails trying to write undefined to the file.
There was a problem hiding this comment.
How about a case where a language used to have translations, but then the translations became outdated because the source messages changed, such that the language is no longer translated? In such a case, I think we would still want to write {} to ${locale}.json. Otherwise, the outdated translations would remain.
I don't really understand how this problem comes up given that bin/transifex/destructure.js is only iterating over locales with a corresponding transifex_*.json file. I think I understand that we need to handle the null case, but I feel like the way to do that is by writing {}.
There was a problem hiding this comment.
I added this because forms doesn't support all the languages that central does.
I don't really understand how this problem comes up given that bin/transifex/destructure.js is only iterating over locales with a corresponding transifex_*.json file.
Just to add a little more here, I'm wondering: writeTranslations() is currently passed a locale argument. So what is passing it an unexpected/nonexistent locale? Shouldn't apps/forms/transifex/ only contain locales supported by Web Forms, not all those supported by Central?
There was a problem hiding this comment.
I'm also happy to continue discussing this in a follow-up issue.
There was a problem hiding this comment.
Circling back to this, I wonder if this has less to do with mismatching sets of supported locales and more to do with the absence of messages outside components. That absence isn't something that happens in Central, so the script might not handle that case well.
There was a problem hiding this comment.
That's exactly the problem. Because web-forms doesn't have any it doesn't need files in the src/locales dir and it feels silly to add a blank one per locale. This will probably change in future but this feels like a sensible guard regardless.
| @@ -0,0 +1 @@ | |||
| {} No newline at end of file | |||
There was a problem hiding this comment.
Currently there are no non-component translations in the forms app.
There was a problem hiding this comment.
I decided not to move this file, but welcome to review feedback. It should probably go in the root/bin but because the web-forms package has a completely different approach it might be confusing there too.
There was a problem hiding this comment.
The root bin/ makes sense to me too. Maybe we could add code comments to try to reduce the confusion. Or what if we added a new bin/ directory to apps/?
I'm also happy to leave it in apps/central/ and see how that feels over time. We can always change it later.
There was a problem hiding this comment.
I'm also happy to leave it in apps/central/ and see how that feels over time. We can always change it later.
This is my preferred approach. I'm hoping the two solutions standardise over time at which point the winning solution can end up at the root.
| @@ -0,0 +1 @@ | |||
| {} | |||
There was a problem hiding this comment.
Currently the forms app only has component translations.
There was a problem hiding this comment.
Why does this file have the .json5 extension?
There was a problem hiding this comment.
In Central, we add comments for translators to the JSON, as JSON comments. Vue I18n understands JSON5, so we use JSON5 in order to leverage JSON comments. restructure.js then converts the Vue I18n JSON-with-comments to structured JSON that Transifex understands.
| "formNotFound": "未找到与此URL对应的表单，请仔细核对。" | ||
| }, | ||
| "zh-Hant": { | ||
| "formNotFound": "此 URL 未找到表單，請仔細檢查。" |
There was a problem hiding this comment.
The web-forms transifex project has fewer languages than central. It's a shame to lose translations but we can add these back from history if we support these languages later.
There was a problem hiding this comment.
Creating them in Transifex is really straightforward. Would you like me to go ahead and add these translations there?
Even if the translations aren't 100% complete across the entire WF, it could still be a great way for anyone to contribute easily, since support is already underway.
There was a problem hiding this comment.
If it's not too much effort to preserve those translations, that sounds nice to me!
There was a problem hiding this comment.
Some of the translation values changed, for example, the modals now have a head and a body not just a head, so there are about 3 that need to change in the unsupported languages. However, I've created the languages and added them to transifex. Do you think I should add them back as enabled languages in i18n.ts or should we wait till we've had the translations filled out and reviewed?
There was a problem hiding this comment.
At least for Central, there's no process for reviewing translations. Generally, we add a new language as soon as it's fully or almost fully translated. Since these translations existed before, my instinct is to add them back as enabled languages, though maybe that doesn't make sense if they're missing a ton of translations.
There was a problem hiding this comment.
I tried to take a look at Transifex, but it's not loading for me at the moment. If leaving these translations and pushing them to Transifex, I wonder whether that creates them all automatically on the remote or just adds English
There was a problem hiding this comment.
There are about 3 missing, but they are all examples where the title got split into a title and body. For now the body is empty and the title has both pieces of information. This will be a little ugly (a long message crammed into a modal header) but I think completely functional.
I'll sync them back...
|
Would you mind reviewing this when you have a chance? |
There was a problem hiding this comment.
When I was last updating Central's translations, I got a permissions error when running tx pull because I don't have access to the Web Forms project in Transifex. I think that's fine, I don't feel like I need that access, but it made me wonder how best to approach that step. To make progress, I temporarily removed Web Forms from .tx/config.
I like what you're doing below with tx pull -r web_forms.app. Maybe we should do something similar for Central, adding a command to Central's package.json.
Another idea is that we could have multiple .tx/config files in different directories.
There was a problem hiding this comment.
Because we release together now I think it'd make sense for all the translations to be able to be updated together which means whoever's doing the release needs permissions on all projects. I'll follow up with Hélène.
There was a problem hiding this comment.
I was wondering about that now that we have a shared timeline/release. I could see it being useful to update all the translations together.
There was a problem hiding this comment.
Did anything change in this file other than going from .js to .ts? Could that go in its own PR?
There was a problem hiding this comment.
I tried to get git to treat this change as a mv so gh will show the diff, but git still thinks the files are different enough to treat them as a delete and add. Sorry!
In summary these are the changes...
- Quite a lot of code got deleted. In particular, I removed the pluralization rules and number formatting which are not needed for this app. I also deleted unsupported languages, and cleaned up functions this app doesn't use.
- Because there was so much stuff unused I could simplify the API somewhat.
- Some typescript sugar was added.
- Finally the way the user selected locale was modified so it now resolves in order: a) the language the user selected in web forms, b) the language the user selected in central, c) the browser language. This feels correct so that any single URL is (as much as possible) a consistent language for the various strings being displayed.
Some of these could be separated into a new PR but it feels like the right time to remove unused functionality, which is most of this change.
I think enough has changed that I'd just review the new file on its own and ignore the deleted one.
There was a problem hiding this comment.
For Central, I usually have separate PRs for logic/process changes and for actual changes to translation text. What do you think about following that pattern here and moving these changes to translation text to their own PR? I just feel like it makes the diff more scannable.
There was a problem hiding this comment.
These changes (dropping languages) are the result of running the new process, so my feeling was it was best to get it to a consistent state, and then do any translation text updates during release. I think there are no actual translation text changes, just removing unsupported languages and fixing broken json.
|
@garethbowen did you get any errors when running destructure.js for Web Forms? There are some issues with it for Central that we're working to address, but they might not affect Web Forms. I'll get on top of closing those issues out especially now that Web Forms will be using destructure.js. |
|
@garethbowen I've left some comments, but things are looking reasonable to me. I'm happy for the Web Forms team to do whatever is easiest in terms of i18n and would welcome you all to the restructure.js/destructure.js party. |
In some places, I feel like Vue I18n isn't adding all that much, but the rich text feature ("component interpolation") is one area that I think Vue I18n does really well. Doesn't the web-forms package have components though? Couldn't it be the case that the package might need the rich text feature one day in the future? I guess I'm not fully understanding the app/package distinction here. If the package will eventually need an answer for rich text, that might point toward a different solution. I'm happy to run with whatever works best for you and Jenny, and I'm happy for Central and Web Forms to be more aligned here, mostly I'm just curious to learn more about why this doesn't come up for the package. |
No, everything ran surprisingly smoothly! I thought I'd have to make changes for running it in another directory but that just worked.
Mostly I'm glad that this change didn't end up being a third translation solution. Getting down from 2 to 1 would be a nice improvement one day.
Components, yes. The bit that doesn't work is when the translation ends up being a function and not a plain string. When this happens you either need JIT compilation or pre-compilation. So far the web-forms packages are just using plain strings with basic interpolation - no HTML (eg: links) to render within the translation text, and no nested translation. In order the keep the web-forms bundles smaller, it has its own translation function which does only what's needed. It's quite possible that WF will need this functionality sooner or later, at which point it might well make sense to use vue-18n and align with the central way. There is actually very little in the WF package that needs translation, because most of the text is part of the form definition itself, or the 3rd party widgets. |
| if (locale) { | ||
| loadLocale(i18n.global, locale); | ||
| } | ||
| loadUsersLocale(); |
There was a problem hiding this comment.
Web Forms has a specific way of determining the language. Since much of the text here is about submission and form rendering, could you create a ticket so that apps/forms uses Web Forms' way to determine the language?
There was a problem hiding this comment.
The main difference between the web-forms app and component is that the app doesn't currently have access to the form languages. It would be possible to parse the XML and figure that out but I don't think it's worth it.
I do read the web-forms local store value and use that above all else so if that is set then they will be in sync.
The downside is they won't be in sync on first load because web-forms app loads before the component.
It would be possible to have the component emit when the language is set (including first run) so the app can listen for that and update the selected locale when that happens. Is that better?
There was a problem hiding this comment.
This seems like more of a question for @latin-panda, but it makes sense to me for the app to listen for the component to emit an event about the locale being set.
Is any text/translations shown in the app before the component is rendered? Maybe for certain error messages? Just wondering if there are any cases where the app needs to figure out the locale without the component telling it.
Are there any cases where the Web Forms app should use or default to the locale that's been set in Central? That's also set in local storage.
There was a problem hiding this comment.
Yes, it would be great to listen for changes when the user selects a form language. That can be a separate ticket.
If the form fails to load for some reason, it can default to localStorage or English so that the error messages are displayed translated
There was a problem hiding this comment.
Are there any cases where the Web Forms app should use or default to the locale that's been set in Central? That's also set in local storage.
Yes! The implementation in this PR uses...
- The web-forms locale set in local storage
- The central locale set in local storage
- The browser navigator.languages
- English
There was a problem hiding this comment.
Yes, it would be great to listen for changes when the user selects a form language. That can be a separate ticket.
Issue raised: getodk/web-forms#855
| "formNotFound": { | ||
| "string": "Unable to open form" | ||
| }, | ||
| "formNotFound.body": { |
There was a problem hiding this comment.
This is a 3-level nested object, with each property using a different naming style: all lowercase, title case, or with a dot separating words (this line). Is this expected or is it an error in parsing?
(I prefer WF structure. 1-level nesting, standard property names)
There was a problem hiding this comment.
In Central, we generally use title case / camelCase. I think we should probably avoid symbols in keys other than _. I think Vue I18n might get confused by a key like formNotFound.body, since it also uses .. (Maybe not, but I'd be cautious.) The way we would typically do this in Central is to make formNotFound an object, then nest body as a message/property within it. If this change were made in the component, restructure.js will update the structure of transifex/strings_en.json accordingly.
(I prefer WF structure. 1-level nesting, standard property names)
Interesting, I hadn't seen that before! I don't mind the WF structure, but I also wouldn't say that I actively prefer it over Central's nested JSON. I do like that the structure of transifex/strings_en.json roughly matches the structure of the Vue I18n JSON.
There was a problem hiding this comment.
@matthew-white Is there a rule for choosing between camelCase and capitalizing the first letter? Maybe each is used for specific things?
There was a problem hiding this comment.
Translations are organized by component name (under the large component object). So when the first letter is capitalized, that's probably a component.
There was a problem hiding this comment.
But that part is done automatically by restructure.js. When developers choose a message key, they should just use camelCase.
There was a problem hiding this comment.
I chose to change as little as possible. For example, I didn't update "formNotFound" to "formNotFound.title". But it seems I've inadvertently created a 3rd naming system by including the ..
Firstly, do you both think it's worth changing keys, or is consistency worth the unconventional system?
Secondly, which naming system should I standardise with? I'm leaning towards the Central naming system, because then it'll be guaranteed to work seamlessly with destructure.
There was a problem hiding this comment.
do you both think it's worth changing keys, or is consistency worth the unconventional system?
The . in the key definitely makes me nervous, since it has special meaning in Vue I18n. That said, things seem to be working. I probably wouldn't change it as part of this PR, but instead wait to do so in a follow-up PR if we decide we want to remove the ..
which naming system should I standardise with? I'm leaning towards the Central naming system, because then it'll be guaranteed to work seamlessly with destructure.
I'm honestly a little surprised (but glad!) that it works as-is with destructure/restructure. I think certain things probably wouldn't work, like @transifexKey comments, which take a .-delimited path.
If one goal of this PR is consistency between the Central and Web Forms apps, I think that points toward the Central naming system. At least removing the .. If you prefer to keep the strings mostly 1-level (not nested), that should be compatible with the Central system, just maybe using a different delimiter than ..
| "body": { | ||
| "full": { | ||
| "string": "Please log in {here} in a different browser tab and try again.", | ||
| "developer_comment": "{here} is a separate string that will be translated below. Its text will be formatted within ODK Central, for example, it might be bold or a link. Its text is:\n\nhere" |
There was a problem hiding this comment.
| "developer_comment": "{here} is a separate string that will be translated below. Its text will be formatted within ODK Central, for example, it might be bold or a link. Its text is:\n\nhere" | |
| "developer_comment": "{here} is a separate string that will be translated below. Its text will be formatted within ODK Central, for example, it might be bold or a link." |
I didn't understand that last piece. Is it a parsing error?
There was a problem hiding this comment.
It's not a parsing error; the comment is autogenerated. It's related to the use of Vue I18n component interpolation. Due to that, {here} will show up as a variable in Transifex. It's translated in a separate message right below. When Vue I18n component interpolation is used, we break the individual parts into separate messages. That way, translators don't have to use Markdown or other formatting when translating. We then link the messages together / contextualize them by adding these autogenerated translator comments.
That said, if you can think of a way to improve the wording of the comments, that would definitely be welcome. This may be a confusing case, because here is both the variable name and the full source text. But in many other cases, those two things aren't the same.
| "lint": "npm-run-all eslint transifex:lint", | ||
| "lint:fix": "npm-run-all eslint:fix transifex:fix", | ||
| "test": "vitest run", | ||
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", |
There was a problem hiding this comment.
The other approach I attempted was to align with the process used in the web-forms package. It works using formatjs which is more lightweight than vue-intl. However it lacks the rich text feature because it doesn't precompile the locale, so couldn't handle HTML elements the app requires
I'm interested in understanding this case better and experimenting. Could you please point me to the code in the component?
There was a problem hiding this comment.
This is a good example:
central-frontend/apps/forms/src/components/web-form-renderer.vue
Lines 326 to 330 in 78b49bc
Here you have a translation string, with some HTML inside it, and another translated string inside that.
There was a problem hiding this comment.
Thank you! I'm going to look at it in more detail
There was a problem hiding this comment.
I'm thinking I should wait for @latin-panda to take a look at this important use case and to consider the overall approach. Does that sound right? Or should I proceed with review with an eye toward trying to merge soon?
There was a problem hiding this comment.
Okay! I should be able to take a closer look tomorrow. I plan to explore how we can support this using the translation library used by web forms.
I originally didn't mean to block the PR, but if there's still plenty of time before the release... :)
There was a problem hiding this comment.
I confirm that this ⬆️ addresses all translation cases needed for apps/forms. I have a POC branch and a unit test covering the key cases (POC translations).
We just need to host the library in a common location accessible to both apps/forms and packages/web-forms. Overall, I recommend this approach; it is straightforward, no big effort, and keeps the translation solution lightweight and up to date with the latest Message Format, without complex parsing or wrappers
@garethbowen, can we reframe this issue getodk/web-forms#855 to focus on using this approach combined with the WF method for determining the language, which is already part of the library?
There was a problem hiding this comment.
I think the refactor POC and that issue are better kept separate, but it depends how everything comes together. For example, if the user selects a language then you don't need an event if and only if the bundling is done in such a way that both the app and component shared the same instance of the translation library. Bundling is very fragile and hard to spot when it breaks so it may need an e2e regression test to ensure the app and the component languages stay in sync.
Even if that is true for our use case, for 3rd party users of the component they may have a completely separate translation library for their app, so they will still benefit from the event. In that case I would keep 855 around as a low priority, and have a new issue for the refactor that can be solved now.
Either way before the regression work is done I think it would be good to have a discussion with someone from Central, perhaps you me and Matt, because ultimately we should be aiming for all of central-frontend to standardise on one approach or the other. There's no point switching the forms app as in your POC, and then turning around to switching back to vue-intl if we decide that's the right approach later, right?
There was a problem hiding this comment.
Happy to have that conversation with you and Matt. I agree we shouldn't rework it twice. However, apps/forms is still small, and a good opportunity to test the translation library :)
I'd frame the question as "does web-forms' approach hold up as the standard" rather than "should we align on vue-i18n". Looking at what each stack costs today:
- Central's vue-i18n setup has about 1,100 lines of custom transifex.js tooling with a hand-rolled PluralForms class, per-locale pluralization rules in i18n.js (cs/es/fr/ja/pt/zh…), a sentence-separator.vue component to work around component-interpolation whitespace, and a destructure/restructure round-trip because
<i18n>blocks live inside .vue files. - Web-forms' formatjs setup is a ~240-line useLocale.ts plus an ~80-line merge script. Plurals come from CLDR via ICU MessageFormat (no custom rules), sentences stay whole inside one message (no separator hack), and translation files stay in Transifex's native format with no round-trip conversion.
That's a meaningful reduction in custom infrastructure, and it's aligned with the ICU/CLDR standard rather than a library-specific model. So my honest view is that going the other direction would be trading a lighter, standards-based implementation for a heavier custom one. I'd want to see a specific case where vue-i18n comes out ahead before treating it as the default.
The migration cost of apps/central is real and worth sizing, but there are strategies we can consider to keep it manageable (good things to dig into in our conversation). This project isn't in maintenance mode; it's still growing in features, so it's nice for the tech stack to move forward too. Let me know when works!
There was a problem hiding this comment.
Conversation continued on Slack...
There was a problem hiding this comment.
Just adding some thoughts here!
I think it would be good to have a discussion with someone from Central, perhaps you me and Matt
Looking forward to discussing this with @latin-panda on a call soon. 👍
I'd frame the question as "does web-forms' approach hold up as the standard" rather than "should we align on vue-i18n"
I feel like there's probably more than one question or one frame to consider here. To me, it's a complex set of tradeoffs for us to discuss.
Looking at what each stack costs today: Central's vue-i18n setup has about 1,100 lines of custom transifex.js tooling
Writing the tooling was a cost for sure, but we bore most of that cost in 2020. There would also be a cost to switching. To me, sizing that cost is an important question to discuss.
a sentence-separator.vue component to work around component-interpolation whitespace
sentence-separator.vue isn't really related to component interpolation. It's more about combining two separate messages, where one message is conditional. Here's an example of where sentence-separator.vue is helpful, not involving component interpolation:
{{ $t('sentence1') }}
<template v-if="someCondition"><sentence-separator/>{{ $t('sentence2') }}</template>I'd be curious about what a solution to this case would look like in Web Forms.
The migration cost of apps/central is real and worth sizing, but there are strategies we can consider to keep it manageable
I'm sure there are some aspects of the migration that would be quite doable. 👍
One specific question I have is how we would approach existing uses of component interpolation. Would we try to merge existing Central messages, which are currently split up for component interpolation? That would discard existing translations unless we also automated the merge of translations within Transifex. Alternatively, would we try to preserve the current split messages by writing our own version of Vue I18n's <i18n-t> component?
Another thing on my mind is the timing in which translations are loaded. Central has many async components, and it's important that not all translations are loaded at once. Vue I18n gives us that functionality out of the box.
|
@latin-panda I hope it's clear that I'm happy for you and Gareth to do whatever works best for you in terms of i18n for the Web Forms app. When I'm reviewing here, it's just with an eye to checking that the Web Forms app is doing what I'd expect given that it's following the same process as Central. But I feel like that "given" is a higher-level decision for you and Gareth to decide. In the meantime, I'm happy to answer any questions about the Central process or to say what I like and don't like about Vue I18n. |
|
@latin-panda I've modified the translation keys to use the nested style rather so now it's consistent with central. Then I synced all the translations to and from transifex so everything is up to date and the removed languages are available again, albeit missing a few translations. While the discussion about whether to standardise with central or web-forms packages is ongoing I would like to merge this as is because it adds back the functionality that was lost in the monorepo merge. Then I propose we split out the refactor as a separate issue that can be prioritised as we see fit. Does that work for you? |
|
@garethbowen Apologies for the delay! Reviewing and testing this PR is my top priority tomorrow |
|
I'll also take a look again as soon as possible, either Tuesday or Wednesday. |
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
Looking good! Just 2 small issues below. I added an update about my experiment.
| if (Object.keys(translations) > 0) { | ||
| fs.writeFileSync( | ||
| `${localesDir}/${locale}.json`, | ||
| JSON.stringify(translations, null, 2) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The Object.keys(translations) > 0 compares an array to a number, which is weird. It can actually use the stringified output, avoid any crash when translations is undefined, and avoid extra work, since the result is reused:
| if (Object.keys(translations) > 0) { | |
| fs.writeFileSync( | |
| `${localesDir}/${locale}.json`, | |
| JSON.stringify(translations, null, 2) | |
| ); | |
| } | |
| const content = JSON.stringify(translations, null, 2); | |
| if (content) { | |
| fs.writeFileSync(`${localesDir}/${locale}.json`, content); | |
| } | |
There was a problem hiding this comment.
Oof. Good spotting. It's not only weird but also a regression because it always returns false.
if(content) doesn't quite work because for an empty object the content is {}
I've gone with if(content !== '{}') because as you say it avoids the extra work, but would be happy to go with if (Object.keys(translations).length > 0) for readability.
There was a problem hiding this comment.
With this fix, or with if (Object.keys(translations).length > 0), running:
npm run transifex:destructure in apps/forms throws this error:
> @getodk/forms@0.1.0 transifex:destructure
> node ../central/bin/transifex/destructure.js
destructuring de
node:internal/errors:540
throw error;
^
TypeError [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received undefined
at Object.writeFileSync (node:fs:2418:5)
at writeTranslations (/Users/jen/Documents/ODK/CODE/CENTRAL/central-frontend/apps/central/bin/util/transifex.js:1092:8)
at Object.<anonymous> (/Users/jen/Documents/ODK/CODE/CENTRAL/central-frontend/apps/central/bin/transifex/destructure.js:25:3)
at Module._compile (node:internal/modules/cjs/loader:1692:14)
at Object..js (node:internal/modules/cjs/loader:1824:10)
at Module.load (node:internal/modules/cjs/loader:1427:32)
at Module._load (node:internal/modules/cjs/loader:1250:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
at Module.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:152:5) {
code: 'ERR_INVALID_ARG_TYPE'
}
Node.js v24.3.0
npm error Lifecycle script `transifex:destructure` failed with error:
npm error code 1
npm error path /Users/jen/Documents/ODK/CODE/CENTRAL/central-frontend/apps/forms
npm error workspace @getodk/forms@0.1.0
npm error location /Users/jen/Documents/ODK/CODE/CENTRAL/central-frontend/apps/forms
npm error command failed
npm error command sh -c node ../central/bin/transifex/destructure.js
npm warn Unknown project config "min-release-age". This will stop working in the next major version of npm.This seems to work: if (content != null && content !== '{}') {
| "lint": "npm-run-all eslint transifex:lint", | ||
| "lint:fix": "npm-run-all eslint:fix transifex:fix", | ||
| "test": "vitest run", | ||
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", |
There was a problem hiding this comment.
I confirm that this ⬆️ addresses all translation cases needed for apps/forms. I have a POC branch and a unit test covering the key cases (POC translations).
We just need to host the library in a common location accessible to both apps/forms and packages/web-forms. Overall, I recommend this approach; it is straightforward, no big effort, and keeps the translation solution lightweight and up to date with the latest Message Format, without complex parsing or wrappers
@garethbowen, can we reframe this issue getodk/web-forms#855 to focus on using this approach combined with the WF method for determining the language, which is already part of the library?
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
It looks good! Thank you!
|
@matthew-white Let me know if you're still planning to have another look through this, or whether it's ok as is. |
|
Sorry, I didn't get to it last week! I'll take another look at it on Tuesday. Or if you're eager to merge, I'd also be happy to file follow-up issues about anything I notice during review so you can merge right away. |
| export const i18n = createI18n({ | ||
| locale: fallbackLocale, | ||
| fallbackLocale, | ||
| messages: { }, |
There was a problem hiding this comment.
Don't you need to load the fallback messages in order for the fallback to work? In Central, we have:
central-frontend/apps/central/src/i18n.js
Line 148 in 695cd61
There was a problem hiding this comment.
Oh I see, there are only component translations currently, so this isn't needed at the moment. en.json5 is empty, so fallbackMessages would also be empty.
| if (locale) { | ||
| loadLocale(i18n.global, locale); | ||
| } | ||
| loadUsersLocale(); |
There was a problem hiding this comment.
Is there something that guarantees that this navigation guard is only called once? I would expect loadUsersLocale() to be called only once at app startup, not whenever the page changes. How about something like:
const removeGuard = router.beforeEach((_to, _from, next) => {
loadUsersLocale();
removeGuard();
next();
});There was a problem hiding this comment.
Hmm yeah not sure what I was thinking. It's sort of moot because there's not really any navigation between pages in web-forms but having it in the router is still weird. I've moved it to happen as part of createI18n
There was a problem hiding this comment.
It's also in the router in Central, but that's because loadLocale() in Central is async (because it sends a request for the locale's top-level/outside-component .json file). Central sends some initial requests in the router so that they're completed before the first page/route is rendered. But I agree that it would be very reasonable for now to call loadUsersLocale() synchronously outside the router / around createI18n.
| "formNotFound": "Unable to open form", | ||
| "formNotFound.body": "Please check that the link is correct. The form may no longer be available, or your access may have expired. If the problem continues, contact the person who sent you the form link.", | ||
| "errorNotProblem": "Something went wrong", | ||
| "errorNotProblem.body": "Please try again later. If the problem continues, contact the person who sent you the form link.", | ||
| "errorNotProblem.status": "Error code: {status}", | ||
| "formNotFound": { | ||
| "title": "Unable to open form", | ||
| "body": "Please check that the link is correct. The form may no longer be available, or your access may have expired. If the problem continues, contact the person who sent you the form link." | ||
| }, | ||
| "errorNotProblem": { | ||
| "title": "Something went wrong", | ||
| "body": "Please try again later. If the problem continues, contact the person who sent you the form link.", | ||
| "status": "Error code: {status}" | ||
| } |
There was a problem hiding this comment.
I didn't review this sort of change closely, but I do support the removal of keys containing .. 👍
There was a problem hiding this comment.
If there are only component translations, does that mean that nothing imports this file? Does it exist just because restructure/destructure explodes without it?
I think it might solve some things to allow destructure.js to generate similar empty .json files. Rather than only creating this single file for the fallback locale and not having any other .json files. It might seem strange to have a directory full of empty .json files, but to me, it would more closely parallel Central. I'm a little surprised to see something that exists for the fallback locale but not any other locale.
| "test": "vitest run", | ||
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", | ||
| "transifex:fix": "node ../central/bin/transifex/restructure.js > transifex/strings_en.json", | ||
| "transifex:lint": "bash -c 'diff transifex/strings_en.json <(node ../central/bin/transifex/restructure.js)'", |
There was a problem hiding this comment.
Thanks. I'll rebase.
There was a problem hiding this comment.
Unfortunately his changes are relative so don't work if ported here.
There was a problem hiding this comment.
Ah interesting. Maybe worth filing an issue about to come back to later? Let me also CC @alxndrsn
There was a problem hiding this comment.
Unfortunately his changes are relative so don't work if ported here.
I don't understand 🤔
There was a problem hiding this comment.
If I copy your changes in the wf apps dir as is then it lints central translations against web-forms translations which fails. I'll have a look at modifying the script sometime...
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", | ||
| "transifex:fix": "node ../central/bin/transifex/restructure.js > transifex/strings_en.json", | ||
| "transifex:lint": "bash -c 'diff transifex/strings_en.json <(node ../central/bin/transifex/restructure.js)'", | ||
| "translations:pull": "cd ../.. && tx pull -r web_forms.app -a -f --mode translator", |
There was a problem hiding this comment.
I would love to get translations:pull for Central as well! 🤩
| "transifex:destructure": "node ../central/bin/transifex/destructure.js", | ||
| "transifex:fix": "node ../central/bin/transifex/restructure.js > transifex/strings_en.json", | ||
| "transifex:lint": "bash -c 'diff transifex/strings_en.json <(node ../central/bin/transifex/restructure.js)'", | ||
| "translations:pull": "cd ../.. && tx pull -r web_forms.app -a -f --mode translator", |
There was a problem hiding this comment.
What do you think about transifex:pull rather than translations:pull? Just thinking that it'd be nice to use the same transifex: prefix for everything rather than have some commands be transifex: and some be translations:.
There was a problem hiding this comment.
I was going for consistency with web-forms package. @latin-panda What do you think about the rename for both wf package and app?
There was a problem hiding this comment.
Yes, it's okay to align the command names. Here, I was thinking of making it about the feature name rather than the tool name.
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
I've left a few additional comments, but it's looking good to me!
If there are any comments that seem better to address later, I'm also happy for a follow-up issue to be filed that just links to those comments. I'm happy for this PR to be merged sooner rather than later.
c9a01e6 to
ff8f41c
Compare
4 participants
Closes getodk/web-forms#825
Created as a draft so it's not merged until after release.
What has been done to verify that this works as intended?
Manual testing, and CI.
Why is this the best possible solution? Were any other approaches considered?
The other approach I attempted was to align with the process used in the web-forms package. It works using formatjs which is more lightweight than vue-intl. However it lacks the rich text feature because it doesn't precompile the locale, so couldn't handle HTML elements the app requires. I tried to add this functionality but it would have required either adding JIT compilation to central OR adding a compilation step to the package, either would add bloat just for a few messages in the forms app.
Ultimately because the forms and central app are being built with the same vite config it made more sense to align with the central approach (this PR).
How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
It shouldn't do. It means in future new and updated translations and locales can be added to transifex and updated in the forms app.
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No.